[PATCH] D93986: [clang-format] Add the possibility to align assignments spanning empty lines or comments

[Lukas Barth via Phabricator via cfe-commits]

tinloaf marked 3 inline comments as done.
tinloaf added a comment.

This is now an all-in-one revision, including bit fields, assignments, declarations and macros. I did not reproduce the full "across empty lines, across comments, across empty lines *and* comments" test suite for all four of them, since they all use the same logic (and code, mostly). `AlignConsecutiveAssignments` is tested extensively, `AlignConsecutiveBitFields` and `AlignConsecutiveDeclarations` only have tests for `AlignAcrossEmptyLinesAndComments` (since they delegate to the same `AlignTokens` method), and `AlignConsecutiveMacros` has its own set of tests, since it uses a different implementation (of the same logic, basically).



================
Comment at: clang/docs/ClangFormatStyleOptions.rst:198
 
-**AlignConsecutiveAssignments** (``bool``)
-  If ``true``, aligns consecutive assignments.
+**AlignConsecutiveAssignments** (``AlignConsecutiveAssignmentsStyle``)
+  Style of aligning assignments on consecutive lines.
----------------
MyDeveloperDay wrote:
> tinloaf wrote:
> > MyDeveloperDay wrote:
> > > As you plan to do the other cases won't this introduce
> > > 
> > > `AlignBitFieldStyle`
> > > and
> > > `AlignMacroStyle`
> > > 
> > > why not have 1  `AlignConsecutiveStyle` that can be used for all 3, then you don't need the other enum `TokenAlignmentStyle ` which means you won't need to map from one to the other?
> > Yes, it would. I was under the assumption that every multiple-choice option in the options should have its own enum, for type safety. But yes, that would be way more elegant (and make the diff much smaller). I'll change it!
> we don't make that distinction for those options that use true/false so I'm not sure why enums need to be any different
> 
> it may require some changes to the doc/tools/dump_style.py to ensure it gives meaningful documentation, but some consistency between the values might help a little (especially as we'd like to use the same AlignTokens algortihm)
> 
> You'd end up with 3 functions to map the various styles to you TokenAlignmentStyle anyway, and hence you'd lose the type safety
> 
> To be honest I get a bit annoyed at the use of "None" "Never" "false", "No" anyway, so I don't feel we are in a great place in terms of consistency.
I have unified the type of the style options. I think the solution for the documentation is Okay, though not ideal. Each of the options' docs now starts with an example including the relevant tokens (e.g., bit field ':'s for `AlignConsecutiveBitFields`), but the examples illustrating the individual options always use assignments. I'm not sure how I could work around this using only a single type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93986