wanders marked an inline comment as done.
wanders added a comment.
In D84090#2160411 <https://reviews.llvm.org/D84090#2160411>, @curdeius wrote:
> The changes look good to me in general. I share your doubt though about
> whether a bool flag is sufficient here. We've seen in the past a few times
> that at some time a false/true flag is not enough. I'd rather go for a
> Before/After/Both/None flag (or similar, naming probably should be coherent
> with other flags). But I'm not really aware of the projects/coding styles
> that use bit fields. Maybe a small research on this would be good to confirm
> or infirm a necessity of higher complexity.
Did some basic research in repos I happened to have.
Here I call the different styles: "Neither" - no spaces around colon. "Both"
space on both sides. "Left" space on left side only. "Right" space on right
side only. ("Right" is of course subject to AlignConsecutiveBitField, so if
alignment requires there to be spaces there will be space).
In summary - in the projects I looked at - the uses doesn't seem to be very
consistent, can even use different styles within same struct. Most projects
mixes "Both" and "Neither". "Right" doesn't really seem to be used - it seems
to be typos when it is. "Left" is used in some cases when there are aligment
for consecutive lines. (keep in mind that there almost always is consecutive
lines, a single bitfield in a struct seldom makes any sense)..
There was one case in musl that really made me smile. `include/arpa/nameser.h`
struct HEADER has different bitfield members depending on endianness, for big
endian it uses "Right" and little endian it uses "Left". (
https://git.musl-libc.org/cgit/musl/tree/include/arpa/nameser.h#n324 )
My guess is that no project's coding style document really specifies this
detail (except implicitly for projects where the style is defined as "whatever
clang-format says").
But I think the conclusion is that it probably should be more that true/false.
Pondering about doing it in steps, so first just `BitFieldColonSpacing` with
values `Both` and `Neither` (i.e same functionality as in current patch) and
then add `Before` as an additional option.
- qemu
- mixing "Both" and "Neither"
- emacs
- mixing "Both" and "Neither"
- git
- mixing "Both" and "Neither"
- linux kernel
- mixing "Both" and "Neither"
- (also has a bunch of "unsigned x:1, y:2, z:3", which is interesting and
seems to be missing tests for clang-format)
- Has consecutive aligned variants
- gnupg
- mixing "Both" and "Neither"
- but mostly "Neither"
- Also has one "Right": `agent/agent.h: int raw_value: 1;`
- lwip
- Mostly "Left" combined with aligned consecutive
- Some "Both"
- intel-pcm
- "Both"
- syslog-ng
- "Neither"
- busybox
- "Left" combined with aligned consecutive
- glibc
- mixing "Both" and "Neither"
- gcc
- mixing "Both" and "Neither" (mostly "Both")
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84090/new/
https://reviews.llvm.org/D84090
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
