[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/docs/ClangFormatStyleOptions.rst

.. code-block:: c++
     void f(int aaaaaaaaaaaaaaaaaaaa, int aaaaaaaaaaaaaaaaaaaa,
            int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}
 
     false:
     void f(int aaaaaaaaaaaaaaaaaaaa,
            int aaaaaaaaaaaaaaaaaaaa,
            int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}
 
+**BitFieldColonSpacing** (``BitFieldColonSpacingStyle``)
+  The BitFieldColonSpacingStyle to use for bitfields.
+
+  Possible values:
+
+  * ``BFCS_Both`` (in configuration: ``Both``)
+    Add one space on each side of the ``:``
+
+    .. code-block:: c++
+
+      unsigned bf : 2;
+
+  * ``BFCS_None`` (in configuration: ``None``)
+    Add no space around the ``:`` (except when needed for
+    ``AlignConsecutiveBitFields``).
+
+    .. code-block:: c++
+
+      unsigned bf:2;
+
+  * ``BFCS_Before`` (in configuration: ``Before``)
+    Add space before the ``:`` only
+
+    .. code-block:: c++
+
+      unsigned bf :2;
+
+  * ``BFCS_After`` (in configuration: ``After``)
+    Add space after the ``:`` only (space may be added before if
+    needed for ``AlignConsecutiveBitFields``).
+
+    .. code-block:: c++
+
+      unsigned bf: 2;
+
+
+
 **BraceWrapping** (``BraceWrappingFlags``)
   Control of individual brace wrapping cases.
 
   If ``BreakBeforeBraces`` is set to ``BS_Custom``, use this to specify how
   each individual brace case should be handled. Otherwise, this is ignored.
 
   .. code-block:: yaml
 

clang/docs/ReleaseNotes.rst

 AST Matchers
 ------------
 
 - ...
 
 clang-format
 ------------
 
-- ...
+- Option ``BitFieldColonSpacing`` has been added that decides how
+  space should be added around identifier, colon and bit-width in
+  bitfield definitions.
+
+  .. code-block:: c++
+
+    // Both (default)
+    struct F {
+      unsigned dscp : 6;
+      unsigned ecn  : 2; // AlignConsecutiveBitFields=true
+    };
+    // None
+    struct F {
+      unsigned dscp:6;
+      unsigned ecn :2;
+    };
+    // Before
+    struct F {
+      unsigned dscp :6;
+      unsigned ecn  :2;
+    };
+    // After
+    struct F {
+      unsigned dscp: 6;
+      unsigned ecn : 2;
+    };
+
 
 libclang
 --------
 
 - ...
 
 Static Analyzer
 ---------------

clang/include/clang/Format/Format.h

   /// Lambdas will not be affected. Only the first ``[`` will get a space added.
   /// \code
   ///    true:                                  false:
   ///    int a [5];                    vs.      int a[5];
   ///    int a [5][5];                 vs.      int a[5][5];
   /// \endcode
   bool SpaceBeforeSquareBrackets;
 
+  /// Styles for adding spacing around ``:`` in bitfield definitions.
+  enum BitFieldColonSpacingStyle {
+    /// Add one space on each side of the ``:``
+    /// \code
+    ///   unsigned bf : 2;
+    /// \endcode
+    BFCS_Both,
+    /// Add no space around the ``:`` (except when needed for
+    /// ``AlignConsecutiveBitFields``).
+    /// \code
+    ///   unsigned bf:2;
+    /// \endcode
+    BFCS_None,
+    /// Add space before the ``:`` only
+    /// \code
+    ///   unsigned bf :2;
+    /// \endcode
+    BFCS_Before,
+    /// Add space after the ``:`` only (space may be added before if
+    /// needed for ``AlignConsecutiveBitFields``).
+    /// \code
+    ///   unsigned bf: 2;
+    /// \endcode
+    BFCS_After
+  };
+  /// The BitFieldColonSpacingStyle to use for bitfields.
+  BitFieldColonSpacingStyle BitFieldColonSpacing;
+
   /// Supported language standards for parsing and formatting C++ constructs.
   /// \code
   ///    Latest:                                vector<set<int>>
   ///    c++03                          vs.     vector<set<int> >
   /// \endcode
   ///
   /// The correct way to spell a specific language version is e.g. ``c++11``.
   /// The historical aliases ``Cpp03`` and ``Cpp11`` are deprecated.
return AccessModifierOffset == R.AccessModifierOffset &&
            SpacesBeforeTrailingComments == R.SpacesBeforeTrailingComments &&
            SpacesInAngles == R.SpacesInAngles &&
            SpacesInConditionalStatement == R.SpacesInConditionalStatement &&
            SpacesInContainerLiterals == R.SpacesInContainerLiterals &&
            SpacesInCStyleCastParentheses == R.SpacesInCStyleCastParentheses &&
            SpacesInParentheses == R.SpacesInParentheses &&
            SpacesInSquareBrackets == R.SpacesInSquareBrackets &&
            SpaceBeforeSquareBrackets == R.SpaceBeforeSquareBrackets &&
+           BitFieldColonSpacing == R.BitFieldColonSpacing &&
            Standard == R.Standard && TabWidth == R.TabWidth &&
            StatementMacros == R.StatementMacros && UseTab == R.UseTab &&
            UseCRLF == R.UseCRLF && TypenameMacros == R.TypenameMacros;
   }
 
   llvm::Optional<FormatStyle> GetLanguageStyle(LanguageKind Language) const;
 
   // Stores per-language styles. A FormatStyle instance inside has an empty

clang/lib/Format/Format.cpp

static void enumeration(IO &IO,
     IO.enumCase(Value, "Always", FormatStyle::SBPO_Always);
 
     // For backward compatibility.
     IO.enumCase(Value, "false", FormatStyle::SBPO_Never);
     IO.enumCase(Value, "true", FormatStyle::SBPO_ControlStatements);
   }
 };
 
+template <>
+struct ScalarEnumerationTraits<FormatStyle::BitFieldColonSpacingStyle> {
+  static void enumeration(IO &IO,
+                          FormatStyle::BitFieldColonSpacingStyle &Value) {
+    IO.enumCase(Value, "Both", FormatStyle::BFCS_Both);
+    IO.enumCase(Value, "None", FormatStyle::BFCS_None);
+    IO.enumCase(Value, "Before", FormatStyle::BFCS_Before);
+    IO.enumCase(Value, "After", FormatStyle::BFCS_After);
+  }
+};
+
 template <> struct MappingTraits<FormatStyle> {
   static void mapping(IO &IO, FormatStyle &Style) {
     // When reading, read the language first, we need it for getPredefinedStyle.
     IO.mapOptional("Language", Style.Language);
 
     if (IO.outputting()) {
       StringRef StylesArray[] = {"LLVM",   "Google", "Chromium", "Mozilla",
                                  "WebKit", "GNU",    "Microsoft"};
static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("SpacesInContainerLiterals",
                    Style.SpacesInContainerLiterals);
     IO.mapOptional("SpacesInCStyleCastParentheses",
                    Style.SpacesInCStyleCastParentheses);
     IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses);
     IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
     IO.mapOptional("SpaceBeforeSquareBrackets",
                    Style.SpaceBeforeSquareBrackets);
+    IO.mapOptional("BitFieldColonSpacing", Style.BitFieldColonSpacing);
     IO.mapOptional("Standard", Style.Standard);
     IO.mapOptional("StatementMacros", Style.StatementMacros);
     IO.mapOptional("TabWidth", Style.TabWidth);
     IO.mapOptional("TypenameMacros", Style.TypenameMacros);
     IO.mapOptional("UseCRLF", Style.UseCRLF);
     IO.mapOptional("UseTab", Style.UseTab);
     IO.mapOptional("WhitespaceSensitiveMacros",
                    Style.WhitespaceSensitiveMacros);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.SpaceAfterTemplateKeyword = true;
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
   LLVMStyle.SpaceBeforeRangeBasedForLoopColon = true;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
   LLVMStyle.SpaceBeforeSquareBrackets = false;
+  LLVMStyle.BitFieldColonSpacing = FormatStyle::BFCS_Both;
   LLVMStyle.SpacesInAngles = false;
   LLVMStyle.SpacesInConditionalStatement = false;
 
   LLVMStyle.PenaltyBreakAssignment = prec::Assignment;
   LLVMStyle.PenaltyBreakComment = 300;
   LLVMStyle.PenaltyBreakFirstLessLess = 120;
   LLVMStyle.PenaltyBreakString = 1000;
   LLVMStyle.PenaltyExcessCharacter = 1000000;

clang/lib/Format/TokenAnnotator.cpp

if (Right.is(TT_ObjCBlockLParen))
     return true;
   if (Right.is(TT_CtorInitializerColon))
     return Style.SpaceBeforeCtorInitializerColon;
   if (Right.is(TT_InheritanceColon) && !Style.SpaceBeforeInheritanceColon)
     return false;
   if (Right.is(TT_RangeBasedForLoopColon) &&
       !Style.SpaceBeforeRangeBasedForLoopColon)
     return false;
+  if (Left.is(TT_BitFieldColon))
+    return Style.BitFieldColonSpacing == FormatStyle::BFCS_Both ||
+           Style.BitFieldColonSpacing == FormatStyle::BFCS_After;
   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_ObjCMethodExpr))
       return false;
     if (Left.is(tok::question))
       return false;
     if (Right.is(TT_InlineASMColon) && Left.is(tok::coloncolon))
       return false;
     if (Right.is(TT_DictLiteral))
       return Style.SpacesInContainerLiterals;
     if (Right.is(TT_AttributeColon))
       return false;
     if (Right.is(TT_CSharpNamedArgumentColon))
       return false;
+    if (Right.is(TT_BitFieldColon))
+      return Style.BitFieldColonSpacing == FormatStyle::BFCS_Both ||
+             Style.BitFieldColonSpacing == FormatStyle::BFCS_Before;
     return true;
   }
   if (Left.is(TT_UnaryOperator)) {
     if (!Right.is(tok::l_paren)) {
       // The alternative operators for ~ and ! are "compl" and "not".
       // If they are used instead, we do not want to combine them with
       // the token to the right, unless that is a left paren.
       if (Left.is(tok::exclaim) && Left.TokenText == "not")

clang/unittests/Format/FormatTest.cpp

verifyFormat("struct A {\n"
                "      bbbbbbbbbbbbbbbbbbbbbbbbb;\n"
                "};");
   verifyFormat("struct MyStruct {\n"
                "  uchar data;\n"
                "  uchar : 8;\n"
                "  uchar : 8;\n"
                "  uchar other;\n"
                "};");
+  FormatStyle Style = getLLVMStyle();
+  Style.BitFieldColonSpacing = FormatStyle::BFCS_None;
+  verifyFormat("struct Bitfields {\n"
+               "  unsigned sClass:8;\n"
+               "  unsigned ValueKind:2;\n"
+               "  uchar other;\n"
+               "};",
+               Style);
+  verifyFormat("struct A {\n"
+               "  int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:1,\n"
+               "      bbbbbbbbbbbbbbbbbbbbbbbbb:2;\n"
+               "};",
+               Style);
+  Style.BitFieldColonSpacing = FormatStyle::BFCS_Before;
+  verifyFormat("struct Bitfields {\n"
+               "  unsigned sClass :8;\n"
+               "  unsigned ValueKind :2;\n"
+               "  uchar other;\n"
+               "};",
+               Style);
+  Style.BitFieldColonSpacing = FormatStyle::BFCS_After;
+  verifyFormat("struct Bitfields {\n"
+               "  unsigned sClass: 8;\n"
+               "  unsigned ValueKind: 2;\n"
+               "  uchar other;\n"
+               "};",
+               Style);
 }
 
 TEST_F(FormatTest, FormatsNamespaces) {
   FormatStyle LLVMWithNoNamespaceFix = getLLVMStyle();
   LLVMWithNoNamespaceFix.FixNamespaceComments = false;
 
   verifyFormat("namespace some_namespace {\n"
                "class A {};\n"
TEST_F(FormatTest, AlignConsecutiveBitFields) {
   Alignment.AlignConsecutiveBitFields = true;
   verifyFormat("int const a     : 5;\n"
                "int oneTwoThree : 23;",
                Alignment);
 
   // Initializers are allowed starting with c++2a
   verifyFormat("int const a     : 5 = 1;\n"
                "int oneTwoThree : 23 = 0;",
                Alignment);
 
   Alignment.AlignConsecutiveDeclarations = true;
   verifyFormat("int const a           : 5;\n"
                "int       oneTwoThree : 23;",
                Alignment);
 
   verifyFormat("int const a           : 5;  // comment\n"
                "int       oneTwoThree : 23; // comment",
                Alignment);
 
   verifyFormat("int const a           : 5 = 1;\n"
                "int       oneTwoThree : 23 = 0;",
                Alignment);
 
   Alignment.AlignConsecutiveAssignments = true;
   verifyFormat("int const a           : 5  = 1;\n"
                "int       oneTwoThree : 23 = 0;",
                Alignment);
   verifyFormat("int const a           : 5  = {1};\n"
                "int       oneTwoThree : 23 = 0;",
                Alignment);
 
+  Alignment.BitFieldColonSpacing = FormatStyle::BFCS_None;
+  verifyFormat("int const a          :5;\n"
+               "int       oneTwoThree:23;",
+               Alignment);
+
+  Alignment.BitFieldColonSpacing = FormatStyle::BFCS_Before;
+  verifyFormat("int const a           :5;\n"
+               "int       oneTwoThree :23;",
+               Alignment);
+
+  Alignment.BitFieldColonSpacing = FormatStyle::BFCS_After;
+  verifyFormat("int const a          : 5;\n"
+               "int       oneTwoThree: 23;",
+               Alignment);
+
   // Known limitations: ':' is only recognized as a bitfield colon when
   // followed by a number.
   /*
   verifyFormat("int oneTwoThree : SOME_CONSTANT;\n"
                "int a           : 5;",
                Alignment);
   */
 }
CHECK_PARSE("IndentExternBlock: Indent", IndentExternBlock,
               FormatStyle::IEBS_Indent);
   CHECK_PARSE("IndentExternBlock: NoIndent", IndentExternBlock,
               FormatStyle::IEBS_NoIndent);
   CHECK_PARSE("IndentExternBlock: true", IndentExternBlock,
               FormatStyle::IEBS_Indent);
   CHECK_PARSE("IndentExternBlock: false", IndentExternBlock,
               FormatStyle::IEBS_NoIndent);
 
+  Style.BitFieldColonSpacing = FormatStyle::BFCS_None;
+  CHECK_PARSE("BitFieldColonSpacing: Both", BitFieldColonSpacing,
+              FormatStyle::BFCS_Both);
+  CHECK_PARSE("BitFieldColonSpacing: None", BitFieldColonSpacing,
+              FormatStyle::BFCS_None);
+  CHECK_PARSE("BitFieldColonSpacing: Before", BitFieldColonSpacing,
+              FormatStyle::BFCS_Before);
+  CHECK_PARSE("BitFieldColonSpacing: After", BitFieldColonSpacing,
+              FormatStyle::BFCS_After);
+
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
   Style.ForEachMacros.clear();
   std::vector<std::string> BoostForeach;
   BoostForeach.push_back("BOOST_FOREACH");
   CHECK_PARSE("ForEachMacros: [BOOST_FOREACH]", ForEachMacros, BoostForeach);
   std::vector<std::string> BoostAndQForeach;
   BoostAndQForeach.push_back("BOOST_FOREACH");