[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

Daniel Jasper via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Apr 20 00:47:15 PDT 2018


djasper added inline comments.


================
Comment at: include/clang/Format/Format.h:154
 
+  /// \brief If a function call, initializer list, or template
+  /// argument list doesn't fit on a line, allow putting all
----------------
writing just "initializer list" is confusing, especially next to the constructor initializer list below. Maybe "brace initializer list"?

Also, if this influences initializer lists and template argument lists, please add tests for those.

If you change this file, please run docs/tools/dump_format_style.py to update the docs.

(also, why is this comment so narrow?)


================
Comment at: include/clang/Format/Format.h:171
+
+  /// \brief If a constructor initializer list doesn't fit on a line, allow
+  /// putting all initializers onto the next line, if
----------------
I think this comment is a bit confusing. The "initializer list" does fit on one line.


================
Comment at: lib/Format/ContinuationIndenter.cpp:757
+        Previous.is(TT_DictLiteral) ||
+        (!Style.AllowAllArgumentsOnNextLine && !State.Line->MustBeDeclaration))
       State.Stack.back().BreakBeforeParameter = true;
----------------
nitpick: move this up one line so it's next to the case for AllowAllParametersOfDeclarationOnNextLine.


================
Comment at: unittests/Format/FormatTest.cpp:3438
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ColumnLimit = 60;
----------------
Only testing this for BCIS_BeforeComma seems a bit bad to me. Is there a reason for it?

Also, I think we should have a test where the constructor declaration itself does not fit on one line, e.g. what's the behavior for:

  Constructor(int param1,
              ...
              int paramN) {
      : aaaaaa(a), bbbbb(b) { .. }


https://reviews.llvm.org/D40988





More information about the cfe-commits mailing list