Page MenuHomeFreeBSD

libalias: Style cleanup
ClosedPublic

Authored by donner on May 14 2021, 1:13 PM.

Details

Summary

libalias is a convolut of various coding styles modified by a series
of different editors enforcing interesting convetions on spacing and
comments.

This patch is a baseline to start with a perfomance rework of
libalias. Upcoming patches should be focus on the code, not on the
style. That's why most annoying style errors should be fixed
beforehand.

Test Plan

Recompile, no functional changes

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

This revision is now accepted and ready to land.May 14 2021, 2:16 PM
sys/netinet/libalias/alias.c
1380–1381

Also leading whitespace issue

/home/emaste/Downloads/D30259.diff:823: space before tab in indent.
                        break;
sys/netinet/libalias/alias_smedia.c
158–159
/home/emaste/Downloads/D30259.diff:3282: space before tab in indent.
                    *ah->sport, *ah->aport, IPPROTO_UDP);

When I applied this change locally to take a look git reported a couple of whitespace issues, identified above.

Then I ran clang-format12 against sys/netinet/libalias/* and put the result in D30260, as part of ongoing efforts to evaluate the possibility of using clang-format for automated code formatting. (No intent to run it over the whole tree, but using it as part of our patch submission process.)

clang-format has a few deficiencies right now and there are some significant things that are just wrong in D30260, but it did identify a few additional style items that can be applied

sys/netinet/libalias/alias_sctp.c
2213–2215

I'd leave these ones unchanged since IMO readability is improved when either both or neither side of an else has braces; style(9) allows either and so I'd argue for leaving it as is.

sys/netinet/libalias/alias_smedia.c
508

I would not drop the braces in these sorts of cases - style no longer specifies no braces for single statement bodies, and given that both the for statement and the loop body are actually spread across multiple source lines I think the braces add clarity

donner added inline comments.
sys/netinet/libalias/alias.c
1380–1381

The problem is not whitespace, but the closing bace. It come a line to early. I'll fix this.

sys/netinet/libalias/alias_sctp.c
2213–2215

I'll roll them back.

sys/netinet/libalias/alias_smedia.c
158–159

Fixed. Thanks.

508

I'll roll them back.

donner marked 4 inline comments as done.
  • Fix balanced braces around "else".
  • Fix some spacing.
This revision now requires review to proceed.May 14 2021, 4:09 PM
  • Remove spaces before tabs.
  • Replace spaces with tabs
  • Incooperate style suggestions from D30260
  • Canonify #ifdef spacing.

I do not take every hint from D30260.
Many of those compiler generated suggestions destroy the flow of reading.
Most of the others are a bit picky on the line length, but this is not reason to change existing code.

I'd be happy to commit this as a base for future work.

This revision is now accepted and ready to land.May 14 2021, 9:33 PM

I do not take every hint from D30260.
Many of those compiler generated suggestions destroy the flow of reading.
Most of the others are a bit picky on the line length, but this is not reason to change existing code.

Indeed - I opened D30260 to show some examples of other things that might be missed, and as an opportunity to see how clang-format compares as this code already had active interest in style(9).

My main goal with clang-format is that we can get to a point where we can tell submitters to just use git clang-format (which formats just the changed lines) to produce an acceptably-well formatted patch.

I'd be happy to commit this as a base for future work.

I agree with committing this and following on with your performance/other changes. I'll leave D30260 around for now if there's more to discuss with others on improving clang-format, but do not intend to commit it.

Thanks for taking this on!

This revision was automatically updated to reflect the committed changes.