daiyousei-qz marked 5 inline comments as done. daiyousei-qz added a comment.
I have also moved the expansion out of the location checking branch. I think it's to find out if a macro actually has a definition which isn't always the case especially for special macros like `__FILE__`. Here's the current presentation. F23848502: image.png <https://reviews.llvm.org/F23848502> F23848497: image.png <https://reviews.llvm.org/F23848497> F23848508: image.png <https://reviews.llvm.org/F23848508> ================ Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:490 + HI.Definition = "#define MACRO 41"; + HI.MacroExpansion = "41"; + }}, ---------------- sammccall wrote: > this is redundant with the definition, ideally I think we'd omit one (the > output is pretty heavyweight). > > It's *almost* correct to omit the expansion for object-like macros, the > problem with this is: > ``` > #define INDIRECT DOUBLE(y) > #define DOUBLE(X) X##X > int INDIRECT; // we *should* show expansion here > ``` > > Still, this is probably rare enough that it's better to not show expansions > for object-like macros on balance? > > (It would be possible to actually check for nested expansions but I doubt > it's worth the complexity, certainly not in this patch) I suppose if we don't have good way to detect nested object-like macro, just leave both definition and expansion is a better idea since people could simply ignore the redundant part. Though I don't have a strong opinion on this and could change that if you really want. 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
