[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Could you please upload a diff with the entire file as context? That makes reviewing this easier.



================
Comment at: docs/ClangFormatStyleOptions.rst:428
 
+**BreakBeforeInhertianceDelimiter** (``boolt``)
+  If ``true``, in the class inheritance expression clang-format will
----------------
Auto-generate this with docs/tools/dump_format_style.py


================
Comment at: include/clang/Format/Format.h:309
+  /// inheritance.
+  bool BreakBeforeInhertianceDelimiter;
+  
----------------
s/Delimiter/Colon/

Not because it is better, but just because it's more consistent with much of the rest of clang-format.


================
Comment at: lib/Format/TokenAnnotator.cpp:950
     bool InCtorInitializer = false;
+    bool InInhertiance = false;
     bool CaretFound = false;
----------------
Maybe "InInheritanceList"?


================
Comment at: lib/Format/TokenAnnotator.cpp:1012
+               Current.Previous->is(TT_InheritanceColon)) {
+      Contexts.back().IsExpression = true;
+      Contexts.back().InInhertiance = true;
----------------
Why would we be in an expression here?


================
Comment at: lib/Format/TokenAnnotator.cpp:2398
 
+// Determine if the next token from the closing scope is an inheritance token
+static bool hasMultipleInheritance(const FormatToken &Tok) {
----------------
I don't understand this comment or what the function is supposed to do. It seems to search whether there is an inheritance comma somewhere in the rest of the line.


================
Comment at: lib/Format/TokenAnnotator.cpp:2490
     return true;
+  //Break only if we have multiple inheritance
+  if (Style.BreakBeforeInhertianceDelimiter &&
----------------
nit: Use periods of the end of sentences and a space after //.


================
Comment at: unittests/Format/FormatTest.cpp:1023
 
+TEST_F(FormatTest, BreakBeforeInhertianceDelimiter) {
+  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
----------------
I am missing tests that show the behavior when there are multiple base classes, but they do fit on one line.


================
Comment at: unittests/Format/FormatTest.cpp:1033
+               StyleWithInheritanceBreak);
+  verifyFormat("class MyClass"
+               "  : public X\n"
----------------
What is this supposed to test?


================
Comment at: unittests/Format/FormatTest.cpp:1039
+  verifyFormat("class MyClass\n"
+               "    : public X // When deriving from more than one class, put "
+               "each on its own\n"
----------------
Sure, but the comment is forcing that, so I don't know what this test does.


Repository:
  rL LLVM

https://reviews.llvm.org/D30487