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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.


================
Comment at: include/clang/Format/Format.h:309
+  /// inheritance.
+  bool BreakInhertianceBeforeColonAndComma;
+  
----------------
Abpostelnicu wrote:
> djasper wrote:
> > Hm. I am still not sure about this flag and it's name. Fundamentally, this is currently controlling two different things:
> > - Whether to wrap before or after colon and comma
> > - Whether or not to force wraps for multiple inheritance
> > 
> > That's going to get us in trouble if at some point people also want other combinations of these two things. I have to alternative suggestions:
> > - Add a single flag (e.g. InheritanceListWrapping) with multiple enum values describing the
> >   various choices.
> > - Don't add a flag at all, but instead break inheritance lists exactly like constructor initializers.
> >   I *think* that should solve all the know use cases for now, is easier to configure for users
> >   and we can cross the bridge of naming additional flags when we have to.
> I see your worries here and indeed the current flags does two things. I'm more inclined to use the num that you suggested since in this way it's easier to separate the coding style differences that somebody uses. 
The trouble is that adding more style options should be done very carefully. Some of the reasons here:
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options

So unless we know that people would want the one style for constructor initializers and the other style for inheritance lists, I'd keep both the flags and the implementation simple.


================
Comment at: lib/Format/TokenAnnotator.cpp:2400
+// Returns 'true' if there is an TT_InheritanceComma in the current token list.
+static bool hasMultipleInheritance(const FormatToken &Tok) {
+  for (const FormatToken* nextTok = Tok.Next; nextTok; nextTok = nextTok->Next)
----------------
Abpostelnicu wrote:
> djasper wrote:
> > I don't think you need this. If you set MustBreakBefore to true for the InheritanceCommas and set NoLineBreak to true when adding the InheritanceColon on the same line, then clang-format will already do the right thing.
> > 
> > Fundamentally, this seems to be identical to how we wrap constructor initializer lists in Mozilla style. So I think we should also implement this the same way (if not even reusing the same implementation).
> Now that i've seen the behaviour of NoLineBreak thanks for pointing it out to me, but still correct me if i'm wrong but shouldn't i use: ``NoLineBreakInOperand``. 
> My guess is if i use NoLineBreak, than breaking on the current line where we would have : or , would be prohibited.
Yes, that would be prohibited and that is intended. Remember that I'd set this after placing the colon on the same line (and I should have written explicitly) as the class name. After that, there must not be any further line breaks.

But again, I think we should just have the exact same behavior and implementation as for constructor initializer lists.


Repository:
  rL LLVM

https://reviews.llvm.org/D30487