[PATCH] D88299: [clang-format] Add MacroUnexpander.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Herald added a project: All.

OK, I think we've reached the point where:

- the impact is very clear, this solves a whole class of clang-format's biggest problems
- the idea clearly works (there are good tests)
- the implementation is very well-documented: I can't really improve my understanding further by asking for things to be better explained
- I can't make clear suggestions to simplify - you've applied all my low-level suggestions and my high-level understanding is poor
- I still don't feel like I really understand how it works, but that's not really different than the other big pieces of clang-format

So I think all that's left to do here is ship it. It makes me uneasy that the core of clang-format is functionally magic (could anyone other than you and Daniel reproduce it after nuclear apocalypse?) but this doesn't really change that state.



================
Comment at: clang/lib/Format/MacroCallReconstructor.cpp:53
+  assert(State != Finalized);
+  LLVM_DEBUG(llvm::dbgs() << "Unex: new line...\n");
+  forEachToken(Line, [&](FormatToken *Token, FormatToken *Parent, bool First) {
----------------
if you want to keep these LLVM_DEBUGs, maybe this should be "MCR" or so instead of "unex"?


================
Comment at: clang/lib/Format/MacroCallReconstructor.cpp:62
+  finalize();
+  UnwrappedLine Final =
+      createUnwrappedLine(*Result.Tokens.front()->Children.front(), Level);
----------------
you might want an assertion that Result has one token with one child (it's pretty obvious in finalize() but less so here)


================
Comment at: clang/lib/Format/MacroCallReconstructor.cpp:98
+       ActiveExpansions.size() != Token->MacroCtx->ExpandedFrom.size())) {
+    if (reconstructActiveCallUntil(Token))
+      First = true;
----------------
nit: I think this would be clearer by naming the result:
`if (bool PassedMacroComma = reconstruct...)`

(because it's not clear from the name what the function returns, and documenting it would help only a little)


================
Comment at: clang/lib/Format/MacroCallReconstructor.cpp:108
+    appendToken(Token);
+  } else {
+    // Otherwise add the reconstructed equivalent of the token.
----------------
(this is the else branch of a negated condition, consider swapping the branches to avoid double-negative)


================
Comment at: clang/lib/Format/MacroCallReconstructor.cpp:114
+
+// Adjusts the stack of active reconstructed liens so we're ready to push
+// tokens. The tokens to be pushed are children of ExpandedParent in the
----------------
liens -> lines

(unless there's a *really* weird metaphor going on here!)


================
Comment at: clang/lib/Format/MacroCallReconstructor.cpp:232
+    } else if (!currentLine()->Tokens.empty()) {
+      // FIXME:assert(!currentLine()->Tokens.empty());
+      // Map all hidden tokens to the last visible token in the output.
----------------
this FIXME looks obsolete?


================
Comment at: clang/lib/Format/Macros.h:190
+  /// that needs to be processed to finish an macro call.
+  /// Only when \c finished() is true, \c getResult() can be called to retrieve
+  /// the resulting \c UnwrappedLine.
----------------
nit: getResult()->takeResult() in comment now


================
Comment at: clang/lib/Format/Macros.h:201
+  /// Generally, this line tries to have the same structure as the expanded,
+  /// formatted unwrapped lines handed in via \c addLine(), with the exception
+  /// that for multiple top-level lines, each subsequent line will be the
----------------
you could give this a name like "tail form", and then refer to it in docs of MacroCallReconstructor::Result, in MacroCallReconstructor.cpp:482, etc.

Up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88299