and do some manual editing afterwards.
Details
- Reviewers
emaste - Commits
- rG78b5dadb58e2: rtld-elf/i386/reloc.c: apply clang-format
rG3a85aa6a1d89: rtld-elf/amd64/reloc.c: apply clang-format
rG986c96b54b9e: rtld-elf/map_object.c: apply clang-format
rGe3035c52f253: rtld-elf/{amd64,i386}/reloc.c: remove unneeded #ifdef dbg
rG7e2f38311e62: rtld-elf/rtld.c: apply clang-format
rGbf3fbf74d12c: rtld obj_remap_relro(): unindent the loop body
rGdd1d72961b8d: rtld-elf/rtld.c: fix typo in comment
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
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?) |
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. |
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. |
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.