[PATCH] D101868: [clang-format] Adds a formatter for aligning arrays of structs

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks requested changes to this revision.
HazardyKnusperkeks added a comment.
This revision now requires changes to proceed.

I've added a few comments, and I would like to hear the opinion of others regarding the left or right alignment of the elements.



================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:603
                                   State.Column + Spaces + PPColumnCorrection);
-
   // If "BreakBeforeInheritanceComma" mode, don't break within the inheritance
----------------
Please don't remove the empty lines.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:733
+  bool couldBeInStructArrayInitializer() const {
+    const auto end = Contexts.rbegin() + 2;
+    auto last = Contexts.rbegin();
----------------
Capital letter for variables.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:733
+  bool couldBeInStructArrayInitializer() const {
+    const auto end = Contexts.rbegin() + 2;
+    auto last = Contexts.rbegin();
----------------
HazardyKnusperkeks wrote:
> Capital letter for variables.
Where does the 2 come from? How do we know that + 2 is always applicable? There should be an explanation, and an assert.

On another note, I would prefer `std::next()`.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2657-2659
+    if (CurrentToken->is(tok::l_brace)) {
+      ++Depth;
+    } else if (CurrentToken->is(tok::r_brace))
----------------
There are still braces.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2663
+      CurrentToken = CurrentToken->Next;
+      if (CurrentToken != nullptr) {
+        CurrentToken->StartsColumn = true;
----------------
I think if you revert the condition and drop the `else` (because of the `break`) makes it more readable.


================
Comment at: clang/lib/Format/TokenAnnotator.h:35
+  LT_VirtualFunctionDecl,
+  LT_ArrayOfStructInitializer
 };
----------------
Add a trailing comma, so that future additions do not need to change this line.


================
Comment at: clang/lib/Format/WhitespaceManager.h:254
+    auto NetWidth = InitialSpaces;
+    for (auto PrevIter = Start; PrevIter != End; PrevIter++) {
+      // If we broke the line the initial spaces are already
----------------
For iterators prefix increment is really better than postfix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101868