[PATCH] D90232: [clang-format] Formatting constructor initializer lists by putting them always on different lines (update to D14484)

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I'm a little confused between BestFit and Compact

I'm not a massive fan of changing unit tests, just saying.



================
Comment at: clang/docs/ClangFormatStyleOptions.rst:1473
 
-**ConstructorInitializerAllOnOneLineOrOnePerLine** (``bool``)
-  If the constructor initializers don't fit on a line, put each
-  initializer on its own line.
+**ConstructorInitializer** (``ConstructorInitializerKind``)
+  Formatting of the constructor initializer.
----------------
We can't easily remove an option once its been released, I know you have code to handle it, but I think it needs to remain in the documentation and hence the Format.h and somehow be marked deprecated otherwise someone is going to look at the documentation and say, I could of sworn I used to be able to use this setting...


================
Comment at: clang/lib/Format/Format.cpp:401
       StringRef BasedOnStyle;
       IO.mapOptional("BasedOnStyle", BasedOnStyle);
       if (!BasedOnStyle.empty()) {
----------------
Nit:What pre-merge said!


================
Comment at: clang/unittests/Format/FormatTest.cpp:4714
   FormatStyle OnePerLine = getLLVMStyle();
-  OnePerLine.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  OnePerLine.ConstructorInitializer = FormatStyle::CI_OnePerLine;
   OnePerLine.AllowAllParametersOfDeclarationOnNextLine = false;
----------------
if you are setting this here to OnePerLine, then how come in the config if its true we set it to best fit?


================
Comment at: clang/unittests/Format/FormatTest.cpp:5034
   FormatStyle OnePerLine = Style;
-  OnePerLine.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  Style.ConstructorInitializer = FormatStyle::CI_BestFit;
   OnePerLine.AllowAllConstructorInitializersOnNextLine = false;
----------------
I personally think these tests should remain unaltered and it should just work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90232