[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.
This revision is now accepted and ready to land.

I've read back through the previous comments, and I'm slightly struggling to understand the reason for the objections. (other than the normal public style)

I'd tend to be wary of the "if its not in a public style guide, its not coming in" rule despite me understanding the rationale for that stance many people have non-public projects but still have millions of lines of code to support:

Many public projects have often already adopted a .clang-format file anyway and as such their style is defined by only what clang-format currently supports and hence it cannot by definition then define a style that clang-format keeps altering the code away from.

So I baulk at the idea that a rule that introduces a new style can't come in, My rule of thumb is don't mess up anyone's existing code, don't make others pay too much for your feature, help us look after this excellent tool by helping the project (bugs, reviews).

With that in mind, I've gone back to the original documentation for AlignOperands and BreakBeforeBinaryOperators, and I'm afraid I cannot see the logic in the current capabilities that align code nicely with BreakBeforeBinaryOperators turned off

F10398828: image.png <https://reviews.llvm.org/F10398828>

but not nicely with BreakBeforeBinaryOperators turned on

F10398823: image.png <https://reviews.llvm.org/F10398823>

If I understand correctly, this new setting (which will be off by default) will allow the following for those that choose it.

F10398841: image.png <https://reviews.llvm.org/F10398841>

and it may be the OCD in me, but that re-alignment makes me feel happy, for this reason alone I'd give this a LGTM, as long as your willing to help us support this behaviour going forward.

That's my rationale for accepting, plus the authors 2.5 years of rebasing a feature, shows a dedication which I am assuming extends to continued support.

my 2c worth.

(images from https://zed0.co.uk/clang-format-configurator/) - 10.0.0+b452de0



================
Comment at: clang/include/clang/Format/Format.h:187
   /// expressions.
-  ///
-  /// Specifically, this aligns operands of a single expression that needs to be
-  /// split over multiple lines, e.g.:
-  /// \code
-  ///   int aaa = bbbbbbbbbbbbbbb +
-  ///             ccccccccccccccc;
-  /// \endcode
-  bool AlignOperands;
+  OperandAlignmentStyle AlignOperands;
 
----------------
Typz wrote:
> MyDeveloperDay wrote:
> > I think you are missing an entry in the operator== in Format.h
> It is there already, since this is not a new field, but just changing the type of an existing field.
sound good.


================
Comment at: clang/unittests/Format/FormatTest.cpp:4147
                "      >> aaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaa);",
                Style);
 
----------------
Nit: I get nervous when we change tests, can we simply add a new duplicated line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D32478