[clang-format] Add BitFieldColonSpacing option

[wanders]

Summary

This new option allows controlling if there should be spaces around
the ':' in a bitfield declaration.

BitFieldColonSpacing accepts four different values:

  // "Both" - default
  unsigned bitfield : 5
  unsigned bf2      : 5  // AlignConsecutiveBitFields=true

  // "None"
  unsigned bitfield:5
  unsigned bf2     :5

  // "Before"
  unsigned bitfield :5
  unsigned bf2      :5

  // "After"
  unsigned bitfield: 5
  unsigned bf2     : 5

---

[MyDeveloperDay]

please clang-format

clang/include/clang/Format/Format.h

2236

We need to regenerate the ClangFormatStyleOptions.rst from this change

run clang/docs/tool/dump_format_style.py

Please add a release note too.

[MyDeveloperDay]

In principle, this looks ok.

clang/unittests/Format/FormatTest.cpp

12163

you are missing a CHECK_PARSE_BOOL test

[MyDeveloperDay]

you need to make a full contextdiff git diff -U 99999999

[wanders]

• Regenerated rst
• Fixed clang format errors
• Added release note
• Added bool parsing test

[curdeius]

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.

[wanders]

In 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")

[MyDeveloperDay]

Let’s do all 4, None, Both, Left, Right

Neither is an odd word for non English speakers

Also I before e can confuse the slightly dyslexic

[krasimir]

@wanders, thank you for digging through code to get a feeling of the usage!

I agree with others to have an enum option. +1 to None over Neither.
I'd prefer Before / After instead of Left / Right; those seem more prevalent in names and values of existing options.
I'd slightly prefer leaving After / Right out; it seems it's not widely used and that would be one less case to maintain. However I'm OK with keeping it in for consistency.

[wanders]

Renamed option to "BitFieldColonSpacing" and made it an enum of: "Both", "None," "Before", "After"

[MyDeveloperDay]

Nit:clang-format the patch

Here is a Tip as to what I do (In case it helps other):

1. I generate the diff off the staged items so I've already git added all the files I'm working on.

2. then I can do git clang-format, this will fix up any files in the diff that need formatting (you'll need to git add them again if they have)

3. I then check the documentation builds with /usr/bin/sphinx-build -n ./docs ./html (if the review contains rst files)

4. then I do git diff --cached -U999999 > patch_to_submitt.diff

5. I check the patch to ensure I'm not changing the mode of the files with grep -A0 -B2 "new mode" patch_to_submitt.diff

6. and this is the patch_tosubmitt.diff I upload to the review

These steps try to reduce the number of review fails I get based on clang-format issues (all of this is scripted so making a patch is repeatable, quick and easy and all based off whats in my staged area)

Apart from that this patch LGTM (Before and After are good names)

But please clang-format before pushing (including the tests)

[wanders]

In D84090#2162389, @MyDeveloperDay wrote:

2. then I can do git clang-format, this will fix up any files in the diff that need formatting (you'll need to git add them again if they have)

(I use https://github.com/kimgr/git-format ) just need to remember to run it... But created a "prepare-patch" script now so as long as I remember to run that.

3. I then check the documentation builds with /usr/bin/sphinx-build -n ./docs ./html (if the review contains rst files)

Great tip! Thanks

4. then I do git diff --cached -U999999 > patch_to_submitt.diff

5. I check the patch to ensure I'm not changing the mode of the files with grep -A0 -B2 "new mode" patch_to_submitt.diff

6. and this is the patch_tosubmitt.diff I upload to the review

These steps try to reduce the number of review fails I get based on clang-format issues (all of this is scripted so making a patch is repeatable, quick and easy and all based off whats in my staged area)

Thanks for assistance and review. And sorry about the extra noise.

[MyDeveloperDay]

Thanks for assistance and review. And sorry about the extra noise.

No worries on my part about the noise, firstly this was a really nice clean, quick review but also the noise is part of the learning for us all (especially for me!), I'd much rather spend the time helping to build a clang-formatters community like this with a little noise. In my view it is important for us to build a strong community who can help take over the mantel from @djasper and @klimek's amazing work and let them move onto other areas of interest hopefully with the knowledge we won't undo all their good work!

if its OK with you I'd like to keep you on my list of people who I can use as a reviewer, I very much appreciated your analytical approach to determine what was the most used options.

[MyDeveloperDay]

just reopening to formally accept

[riccibruno]

In D84090#2162389, @MyDeveloperDay wrote:

Nit:clang-format the patch

Here is a Tip as to what I do (In case it helps other):

1. I generate the diff off the staged items so I've already git added all the files I'm working on.

2. then I can do git clang-format, this will fix up any files in the diff that need formatting (you'll need to git add them again if they have)

3. I then check the documentation builds with /usr/bin/sphinx-build -n ./docs ./html (if the review contains rst files)

4. then I do git diff --cached -U999999 > patch_to_submitt.diff

5. I check the patch to ensure I'm not changing the mode of the files with grep -A0 -B2 "new mode" patch_to_submitt.diff

6. and this is the patch_tosubmitt.diff I upload to the review

These steps try to reduce the number of review fails I get based on clang-format issues (all of this is scripted so making a patch is repeatable, quick and easy and all based off whats in my staged area)

Apart from that this patch LGTM (Before and After are good names)

But please clang-format before pushing (including the tests)

That's useful, thank you! I was not aware of git clang-format, which unlike clang-format-diff seems to respect .clang-format.

clang/include/clang/Format/Format.h

   /// \endcode
   bool SpaceBeforeSquareBrackets;
 
+  /// If ``false``, space will be removed around ``:`` in bitfield declarations.
+  /// \code
+  ///    true:                                  false:
+  ///    unsigned bf : 3;              vs.      unsigned bf:3;
+  /// \endcode
+  bool SpaceAroundBitFieldColon;
+
   /// Supported language standards for parsing and formatting C++ constructs.
   /// \code
   ///    Latest:                                vector<set<int>>

            SpacesInParentheses == R.SpacesInParentheses &&
            SpacesInSquareBrackets == R.SpacesInSquareBrackets &&
            SpaceBeforeSquareBrackets == R.SpaceBeforeSquareBrackets &&
+           SpaceAroundBitFieldColon == R.SpaceAroundBitFieldColon &&
            Standard == R.Standard && TabWidth == R.TabWidth &&
            StatementMacros == R.StatementMacros && UseTab == R.UseTab &&
            UseCRLF == R.UseCRLF && TypenameMacros == R.TypenameMacros;

clang/lib/Format/Format.cpp

     IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
     IO.mapOptional("SpaceBeforeSquareBrackets",
                    Style.SpaceBeforeSquareBrackets);
+    IO.mapOptional("SpaceAroundBitFieldColon",
+                   Style.SpaceAroundBitFieldColon);
     IO.mapOptional("Standard", Style.Standard);
     IO.mapOptional("StatementMacros", Style.StatementMacros);
     IO.mapOptional("TabWidth", Style.TabWidth);

   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
   LLVMStyle.SpaceBeforeSquareBrackets = false;
+  LLVMStyle.SpaceAroundBitFieldColon = true;
   LLVMStyle.SpacesInAngles = false;
   LLVMStyle.SpacesInConditionalStatement = false;
 

clang/lib/Format/TokenAnnotator.cpp

   if (Right.is(TT_RangeBasedForLoopColon) &&
       !Style.SpaceBeforeRangeBasedForLoopColon)
     return false;
+  if (Left.is(TT_BitFieldColon))
+    return Style.SpaceAroundBitFieldColon;
   if (Right.is(tok::colon)) {
     if (Line.First->isOneOf(tok::kw_case, tok::kw_default) ||
         !Right.getNextNonComment() || Right.getNextNonComment()->is(tok::semi))

       return false;
     if (Right.is(TT_CSharpNamedArgumentColon))
       return false;
+    if (Right.is(TT_BitFieldColon))
+      return Style.SpaceAroundBitFieldColon;
     return true;
   }
   if (Left.is(TT_UnaryOperator)) {

clang/unittests/Format/FormatTest.cpp

                "  uchar : 8;\n"
                "  uchar other;\n"
                "};");
+  FormatStyle LLVMWithNoSpaceAroundBitfields = getLLVMStyle();
+  LLVMWithNoSpaceAroundBitfields.SpaceAroundBitFieldColon = false;
+  verifyFormat("struct Bitfields {\n"
+               "  unsigned sClass:8;\n"
+               "  unsigned ValueKind:2;\n"
+               "  uchar other;\n"
+               "};", LLVMWithNoSpaceAroundBitfields);
 }
 
 TEST_F(FormatTest, FormatsNamespaces) {

                "int       oneTwoThree : 23 = 0;",
                Alignment);
 
+  Alignment.SpaceAroundBitFieldColon = false;
+  verifyFormat("int const a          :5;\n"
+               "int       oneTwoThree:23;",
+               Alignment);
+
   // Known limitations: ':' is only recognized as a bitfield colon when
   // followed by a number.
   /*