clang-format libalias with D30259 applied
Needs ReviewPublic
Actions

Authored by emaste on May 14 2021, 2:33 PM.

Details

Reviewers: None

Summary

For reference, to examine the differences

@donner is fixing style in libalias, in D30259. I applied the patch from D30259 and then ran sys/netinet/libalias/* through clang-format to see if it finds additional issues and continue the evaluation of clang-format's suitability for automated code formatting.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

emaste created this revision.May 14 2021, 2:33 PM

Herald added a subscriber: melifaro. · View Herald TranscriptMay 14 2021, 2:33 PM

emaste requested review of this revision.May 14 2021, 2:33 PM

@donner for context I have been looking at clang-format's output on some sample files, e.g. D26340, and @domagoj.stolfa_gmail.com did the same with uncrustify in D30248.

Thank you for the cool idea! There are some interesting ideas which are worth to be applied.

I don't think that "tabs for aligment" to ease reading large lists of functions should be removed.

emaste mentioned this in D30259: libalias: Style cleanup.May 14 2021, 2:58 PM

Thank you for the input.

Regen after effc8e57fbf03d7f1423c662caf2b63f0d1d21a9

arichardson added a subscriber: arichardson.May 15 2021, 10:48 PM

I read through about half of these. But there's a number of trends:
alined things are messed up, and introduces a lot of churn
the wrapping to make things optimally introduces a lot of churn: some better, some worse, some just different.
single line comments are messed up in the wrapping and at other times
space after queue macros is wrong
space after 'new' is wrong
Slavish enforcement of 80 columns often makes the code a bit harder to read.

On the plus side, the include sorting looked good.

sys/netinet/libalias/alias.c
143	The above shuffling of includes is fine.
321	this is gratuitous in that it was fine before . In this case, I don't think it matters much, but there's many places people put logically similar things together on one line and break with the strictly optimal 80 column packing.
356–367	These, on the other hand are generally more readable, but I'd also be inclined to say that the last couple are fine either way and relaxing 80 columns would result in more readable code.
898	this shouldn't be snugged up a line. The line should end with {
904	this is wrong } belongs on the next line.
1294	The aligned comments is better, imho.
1454	this is ugly and not style(9) compliant
sys/netinet/libalias/alias.h
40	It's unfortunate we can't say both tabs and spaces are good there...
86–94	I really don't like this. But this issue is also present in the gratuitous reformatting of variable declarations.
151	I find the reformatted stuff harder to read. the original columns were better.
sys/netinet/libalias/alias_db.c
89	I think this is gratuitous and inconsistent with how we do things elsewhere, while the old code was consistent.
161	this is a bad choice.
171	all this wrapping makes it harder to read
220	not style(9) compliant
443–444	not consistent with existing practice.
sys/netinet/libalias/alias_util.c
160	this is wrong.

I read through about half of these. But there's a number of trends:
alined things are messed up, and introduces a lot of churn
...

Indeed - my main point isn't that we should run clang-format over the whole tree (at least in the near future), but rather if someone submitted a new file or a patch formatted like this, would it be fine. This directory provided a good sample as it was undergoing style improvements anyway. My hope is that we can eliminate the things clang-format just gets completely wrong.

sys/netinet/libalias/alias.c
321	I agree the churn for this sort of thing is not worth it in isolation.
356–367	but also I think improved readability here is just a side-effect of rewrapping,
898	yes, a little clang-format improvement ought to be made here
1454	the old version wasn't either :) my guess is it won't be possible to teach clang-format that a /* single line comment / should be rewrapped as / a comment like this */ and that we'd just have to fix this manually first. (I assume it would have been left alone if the comment fit in 80 cols, although putting the 1st and 2nd arg on one line probably also can't be fixed)
sys/netinet/libalias/alias.h
86–94	Yeah. If we start going farther with clang-format I would probably suggest we just `/* clang-format off */` around these sorts of things
sys/netinet/libalias/alias_db.c
161	Yeah, `(la->sctpLinkCount >> 1) +` seems better, although is it over 80 cols with the comment? But either way it seems hard to read before or after
220	Yes same case of rewrapping comment over 80 cols. Although, bikesheds be damned, I wish we used /* This sort of style * for multiline comments */ since I think it is just as readable and saves an extra line
sys/netinet/libalias/alias_util.c
160	@arichardson clang-format treating this as a c++ file?

arichardson added inline comments.May 19 2021, 10:09 PM

sys/netinet/libalias/alias_util.c
160	This definitely looks like a bug we should report upstream.

In D30260#681726, @emaste wrote:

I read through about half of these. But there's a number of trends:
alined things are messed up, and introduces a lot of churn
...

Indeed - my main point isn't that we should run clang-format over the whole tree (at least in the near future), but rather if someone submitted a new file or a patch formatted like this, would it be fine. This directory provided a good sample as it was undergoing style improvements anyway. My hope is that we can eliminate the things clang-format just gets completely wrong.

Yea. That's why I so picky. My comments about the churn is because git clang-format would gratuitously reformat the patches and make the style of the file mixed... I understand that this is a science experiment to see what can be used.