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