[PATCH] D63062: [clang-format] Added New Style Rule: BitFieldDeclsOnSeparateLines

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay requested changes to this revision.
MyDeveloperDay added inline comments.
This revision now requires changes to proceed.


================
Comment at: docs/ClangFormatStyleOptions.rst:194
 
+**BitFieldDeclsOnSeparateLines** (``bool``)
+  If ``true``, Align Bitfield Declarations on separate lines.
----------------
Isn't the documentation normally alphabetric? shouldn't it be after AlignConsecutiveAssignments


================
Comment at: include/clang/Format/Format.h:104
 
+  /// If ``true``, Linesup Bitfield Declarations.
+  /// This will lineup Bitfield declarations on consecutive lines. This
----------------
Align


================
Comment at: include/clang/Format/Format.h:112
+  /// \endcode
+  bool BitFieldDeclsOnSeparateLines;
+
----------------
I think this should be alphabetic in this file (not sure though check the rest of it)

are you happy with the name?  BitFieldDeclsOnSeparateLines? 

1) people often spell Separate incorrectly (didn't you?), this could lead to misconfigured
2) isn't this really a ''Break'' rule

I want to say this might better as  something like "BreakAfterBitFieldDecl"




================
Comment at: lib/Format/ContinuationIndenter.cpp:284
+    return true;
   if (!Current.CanBreakBefore && !(State.Stack.back().BreakBeforeClosingBrace &&
                                    Current.closesBlockOrBlockTypeList(Style)))
----------------
something here doesn't feel quite right,, without trying the code change myself I cannot tell, did you ever try this code without having the same clause in canBreak() and mustBreak()? (i.e. just put it in mustBreak)

The reason I ask is I'm unclear as to why the other mustBreak() rules aren't here in canBreak() if thats the case


================
Comment at: lib/Format/TokenAnnotator.cpp:2921
+      Right.is(tok::identifier) && (Right.Next->is(tok::colon)))
+      return true;
   if (Right.NewlinesBefore > 1 && Style.MaxEmptyLinesToKeep > 0)
----------------
mgorny wrote:
> Misindent.
This code appears 3 times (does it need to appear 3 times?), do we need some sort of 

```
bool isBitField(FormatToken)
{
    ...
}
```

Should a bit field check for the existence of a number after the colon? I can't think of other C++ constructs that appear as

```
comma identifier colon
```

but given that clang-format is used for ObjC,ProtoBuf,Java,JavaScript,C# I'm pretty sure something odd is going to happen with JavaScript named parameters, to be honest I think this is going to cause the following to get reformatted 

MyFunctionCall({ xPosition: 20**, yPosition: 50,** width: 100, height: 5, drawingNow: true });


```
MyFunctionCall({ xPosition: 20**, yPosition: 50,**
                            width: 100, 
                            height: 5, 
                            drawingNow: true });
```

or something like that


================
Comment at: unittests/Format/FormatTest.cpp:3671
+  );
+} 
 
----------------
please add a test with comments (it will get logged)


```
unsigned int baz : 11, /*motor control flags*/
                     add: 2    /* control code for turning the lights on */ ,
                     foo: 3     /* (unused */
```



Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63062/new/

https://reviews.llvm.org/D63062