Page MenuHomeFreeBSD

rtld: apply clang-format
ClosedPublic

Authored by kib on Sat, Jan 18, 3:11 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jan 27, 12:15 AM
Unknown Object (File)
Sun, Jan 26, 6:37 AM
Unknown Object (File)
Thu, Jan 23, 7:05 PM
Unknown Object (File)
Thu, Jan 23, 6:21 AM
Unknown Object (File)
Thu, Jan 23, 5:15 AM
Unknown Object (File)
Tue, Jan 21, 1:51 AM
Unknown Object (File)
Mon, Jan 20, 1:44 PM
Unknown Object (File)
Mon, Jan 20, 1:25 PM
Subscribers

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib requested review of this revision.Sat, Jan 18, 3:11 AM

I am happy with any feedback.
The git-blame-ignore-revs file cannot be updated before commit.

Overall I think this is a fine idea. I am curious about the manual fixups you applied (or put another way, how significant a subsequent clang-format would be). I tried running it again over rtld.c and see a bunch of things like

        for (ph = obj->phdr;
-           (const char *)ph < (const char *)obj->phdr + obj->phsize; ph++) {
+            (const char *)ph < (const char *)obj->phdr + obj->phsize; ph++) {

i.e., mine added an extra space to line up under the first char inside for's (). This feels like a clang-format bug to me.

My thinking is that if clang-format get us 95+% of what we want and we can tolerate the remaining 5% there is some value in going with what it chose rather than applying manual fixups.

CC @imp as we've been talking about clang-format before.

I'm definitely happy to move away from the mix of 4 and 8 space indentation.

libexec/rtld-elf/amd64/reloc.c
106

this one is somewhat unfortunate

199

cast on same line? (or, it moved to fit in 80 cols?)

kib marked 2 inline comments as done.Sat, Jan 18, 11:26 PM

Overall I think this is a fine idea. I am curious about the manual fixups you applied (or put another way, how significant a subsequent clang-format would be). I tried running it again over rtld.c and see a bunch of things like

        for (ph = obj->phdr;
-           (const char *)ph < (const char *)obj->phdr + obj->phsize; ph++) {
+            (const char *)ph < (const char *)obj->phdr + obj->phsize; ph++) {

i.e., mine added an extra space to line up under the first char inside for's (). This feels like a clang-format bug to me.

Do you see such cases in the posted patch? I was not able to find it. I changed the ugly-formatted obj_remap_relro() control, and then also de-intended the body (in separate commit).
Note that I used llvm19 clang-format.

My thinking is that if clang-format get us 95+% of what we want and we can tolerate the remaining 5% there is some value in going with what it chose rather than applying manual fixups.

CC @imp as we've been talking about clang-format before.

I'm definitely happy to move away from the mix of 4 and 8 space indentation.

libexec/rtld-elf/amd64/reloc.c
106

De-indent was a technique blessed by bde. This, plus the fact that the string is more greppable after the split is eliminated.

kib marked an inline comment as done.

obj_remap_relro() re-formatting

Do you see such cases in the posted patch?

No, sorry for confusion - I mean I applied your patch then ran clang-format locally and looked at the diff, and my run of clang-format produced that oddity.

Generally, I tend to like the changes. The extra indent forcing extra wrapping on things that are still somewhat simple suggests at least an experiment with 90 or 100 (or 110?) character lines for comparison might be in order.

libexec/rtld-elf/amd64/reloc.c
106

I agree with kib@. It's the typical way to deal with long strings.

112

I think this is a bit too ridged here and elsewhere about forcing this to to lines... I'd be in favor of a relaxed width of 100 or 110 characters since we're ingesting a lot of code with lines that long today and nobody is noticing...

269

here too (longer line) and several other places in these case statements.

In D48509#1106934, @imp wrote:

Generally, I tend to like the changes. The extra indent forcing extra wrapping on things that are still somewhat simple suggests at least an experiment with 90 or 100 (or 110?) character lines for comparison might be in order.

Well, I myself is in the strong opponent of the long lines.

I have several ways to access my workstation. Starting from random (trusted) windows machine with putty which I do not want to configure, and which provides 80x24 xterm emulation. Ending with the X server session on 4k monitor, where two-pane emacs have the natural line limit around 85 columns. In all that cases, 80 cols is large enough for me.