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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
