feg208 added a comment.
To answer the question of why I think this is different then other alignment
options....It seems to me that each alignment option emphasizes a specific
thing, be it macros, bitfields, or (maybe closer in spirit) more simple
declarations and assignments. I think this case is unique and not currently
addressed by any of the existing alignment mechanisms.
================
Comment at: clang/include/clang/Format/Format.h:93
+ /// if ``true``, when using static initialization for an array of structs
+ /// aligns the fields into columns
----------------
curdeius wrote:
> Why static?
I was quoting from an internal ticket but it doesn't make sense. I removed it
================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:32
+// The notion here is that we walk through the annotated line looking for
+// things like static initialization of arrays and flag them
+bool isArrayOfStructuresInit(const AnnotatedLine &Line) {
----------------
HazardyKnusperkeks wrote:
> Dot at the end. :)
One of these days I'll learn the English language. Then watch out!
================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:37
+ const auto *CurrentToken = Line.First;
+ enum class DetectAsiFsm {
+ null,
----------------
HazardyKnusperkeks wrote:
> What stands AsiFsm for?
Aligned Structure Initializer Finite State Machine but honestly this entire
flow was misguided as the suggested tests pointed out. I switched it to
something that fits (I think) a bit better with a wider array of possible inputs
================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1232
+ } else if (CurrentToken->is(tok::r_brace)) {
+ Depth--;
+ }
----------------
HazardyKnusperkeks wrote:
> Why postfix, especially if using prefix in the `if`?
just an oversight. In hindsight it does look goofy ehh?
================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1404
+ isArrayOfStructuresInit(TheLine));
+ if (AlignArrayOfStructuresInit) {
+ Penalty += AlignedArrayOfStructuresInitLineFormatter(
----------------
HazardyKnusperkeks wrote:
> I'm not sure that you should go before the no column limit. In either case
> you should have a test case for that.
I kept the ordering but I will admit to misgivings about it. None of the tests
fail and it seems like it needs to be ordered in this way but as a newbie
contributor to this project I don't feel confident in that assertion. Do the
included tests cover it? What test that isn't currently there would better
create issues?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101868/new/
https://reviews.llvm.org/D101868
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
