[PATCH] [ClangFormat] Add ConstructorInitializerOffset to control initializer list indentation

Klemens Baum klemensbaum at gmail.com
Tue Aug 13 00:45:15 PDT 2013


  a) I don't have commit access, so you can go ahead and commit the patch when it's ready.
  b) I can't seem to upload full-context diffs because phabricator silently truncates FormatTest.cpp.

  I'll upload a new diff with the proper alphabetical orderings; please see if my replies address the other issues.


================
Comment at: ../tools/clang/lib/Format/Format.cpp:654
@@ +653,3 @@
+        State.Column = FirstIndent + Style.ConstructorInitializerIndentWidth;
+        State.Stack.back().Indent = State.Column;
+      } else if (Current.Type == TT_CtorInitializerComma) {
----------------
Daniel Jasper wrote:
> I think this is wrong (needs to be State.Column + 2) and unnecessary (gets overwritten somewhere at the beginning of moveStateToNextToken()). Try just removing this.
Actually, this is necessary for BreakConstructorInitializersBeforeComma. It only gets overwritten when that option is false.

================
Comment at: ../tools/clang/lib/Format/Format.cpp:656
@@ +655,3 @@
+      } else if (Current.Type == TT_CtorInitializerComma) {
+        assert(Style.BreakConstructorInitializersBeforeComma);
+        State.Column = State.Stack.back().Indent;
----------------
Daniel Jasper wrote:
> Do not assert!!! This is easy to force by user input, e.g. by:
> 
>   Constructor::Constructor() : a(a) // a
>   , b(b) {}
> 
> It's fine not to format perfectly in that case, but we shouldn't crash.
Since this whole block is within the `if (Newline)`, `Current` refers to the first token to emit on that line. Unless we are using the BreakConstructorInitializersBeforeComma style, it cannot be an initializer list comma token. So I don't think this can be triggered by user input.

================
Comment at: ../tools/clang/lib/Format/Format.cpp:657
@@ -648,1 +656,3 @@
+        assert(Style.BreakConstructorInitializersBeforeComma);
+        State.Column = State.Stack.back().Indent;
       } else {
----------------
Daniel Jasper wrote:
> If you don't assert, this entire if-branch can be removed.
It cannot be removed because the final else block would fall back to indenting with 4 spaces in cases where `BreakConstructorInitializersBeforeComma = true` and `ConstructorInitializerIndentWidth = 0`, due to the way it detects continuations by checking `State.Column == FirstIndent`.


http://llvm-reviews.chandlerc.com/D1360



More information about the cfe-commits mailing list