nridge added a comment. Thanks for the update!
In D127082#3606746 <https://reviews.llvm.org/D127082#3606746>, @daiyousei-qz wrote: > Therefore, I still put definition in front of the expansion in this update. > > Regarding to the size limit of expansion: > Here I'm using a 2048 bytes buffer. I think these are reasonable choices to start with. We can always tweak them after the fact if we'd like. ================ Comment at: clang-tools-extra/clangd/Hover.cpp:676 + auto Expansion = AST.getTokens().expansionStartingAt(&Tok); + if (Expansion) { + // Use a fixed size buffer for better performance and presentation. ---------------- nit: these two lines can be combined as: ``` if (auto Expansion = AST.getTokens().expansionStartingAt(&Tok)) { ... } ``` ================ Comment at: clang-tools-extra/clangd/Hover.cpp:1091 + + // Reformat Macro Expansion + if (!HI->MacroExpansion.empty()) { ---------------- It would be interesting to have a couple of test cases that exercise the reformatting in non-trivial ways, e.g. long expansions that need to be wrapped onto multiple lines I would suggest two such test cases, one with the expansion being in a declaration context, and the second an expression context (for this one, to make it long enough, the expansion could contain e.g. an `a ? b : c` expression) (I'm suggesting the expression-context testcase in part as a sanity check to make sure that `format::reformat()` handles such code reasonably in the first place) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/ https://reviews.llvm.org/D127082 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
