HazardyKnusperkeks added a comment. I really like alignment! :)
But why is this completely different than the other alignment options? You should also add an entry to the release notes. ================ 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) { ---------------- Dot at the end. :) ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:37 + const auto *CurrentToken = Line.First; + enum class DetectAsiFsm { + null, ---------------- What stands AsiFsm for? ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:82 + } + CurrentToken = CurrentToken->getNextNonComment(); + } ---------------- I would prefer a `for(; CurrentToken != Line.Last && CurrentToken != nullptr; CurrentToken = CurrentToken->getNextNonComment()) ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1232 + } else if (CurrentToken->is(tok::r_brace)) { + Depth--; + } ---------------- Why postfix, especially if using prefix in the `if`? ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1244 + Column = 0; + Row++; + } else { ---------------- Same. ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1276 + } + Layout.emplace_back(std::vector<unsigned>{}); + auto Row = Layout.size() - 1; ---------------- With this you should save yourself the temporary. ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1278 + auto Row = Layout.size() - 1; + Layout[Row].push_back(0); + auto Column = 0U; ---------------- But why not just initialize the vector directly with 1 element of 0? ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1316-1327 + while (true) { + auto MaxColumn = 0U; + for (auto Row = 0U; Row < RowCount; Row++) { + MaxColumn = std::max(MaxColumn, LayoutMatrix[Row][Column]); + } + for (auto Row = 0U; Row < RowCount; Row++) { + LayoutMatrix[Row][Column] = MaxColumn - LayoutMatrix[Row][Column]; ---------------- You should also assert that `LayoutMatrix.size() > 0`. ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1404 + isArrayOfStructuresInit(TheLine)); + if (AlignArrayOfStructuresInit) { + Penalty += AlignedArrayOfStructuresInitLineFormatter( ---------------- I'm not sure that you should go before the no column limit. In either case you should have a test case for that. ================ Comment at: clang/unittests/Format/FormatTest.cpp:16371 + Style); +} + ---------------- curdeius wrote: > I think we need more tests: > * with a comma after the last element of array > * with assignment and not only initialization? > * without `struct` keyword > * with short lines/long values to see how wrapping behaves > * also, with multiple string literals in one struct, e.g. `"hello" "world"` > (that will get splitted into multiple lines) > * with non-plain-array types (i.e. missing `[]`/`[3]`) > * interaction with other Align* options (e.g. `AlignConsecutiveAssignments`, > two arrays one after another, `AlignConsecutiveDeclarations`). In addition to that I want to see: * How it behaves with a smaller column limit which would be violated if there is no additional wrapping. There I also want one without `verifyFormat`. * With no column limit. * Maybe with a `const`, `inline` or something similar. (How about east `const` vs. west `const`?) 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