[PATCH] D148457: [clangd] Support macro evaluation on hover

This revision was automatically updated to reflect the committed changes. Closed by commit rG7385cc389aba: [clangd] Support macro evaluation on hover (authored by zyounan). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148457/new/

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan updated this revision to Diff 520638. zyounan added a comment. Final update Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148457/new/ https://reviews.llvm.org/D148457 Files: clang-tools-extra/clangd/Hover.cpp

[PATCH] D148457: [clangd] Support macro evaluation on hover

nridge accepted this revision. nridge added a comment. This revision is now accepted and ready to land. Thanks! LGTM with one final nit (the other comment is just food for future thought) Comment at: clang-tools-extra/clangd/Hover.cpp:503 + +std::optional printExprValue(const

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan added a comment. Thank you for the opinions. I've updated and please take a look. Comment at: clang-tools-extra/clangd/Hover.cpp:705 + + // If macro expands to one single token, rule out punctuator or digraph. + // E.g., for the case `array L_BRACKET 42 R_BRACKET;`

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan updated this revision to Diff 520555. zyounan marked 2 inline comments as done. zyounan added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148457/new/ https://reviews.llvm.org/D148457 Files:

[PATCH] D148457: [clangd] Support macro evaluation on hover

nridge added a comment. Thanks for the update, and thank you Sam for the suggestions! In D148457#4307402 , @sammccall wrote: > A couple of ideas: > > - special-case alignof > - (generalization) allow partial selection via macros that expand to a single

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan added a comment. Gently ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148457/new/ https://reviews.llvm.org/D148457 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan updated this revision to Diff 518280. zyounan added a comment. Exclude the macro expands to single punctulator token. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148457/new/ https://reviews.llvm.org/D148457 Files:

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan added inline comments. Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3901 +case 1: + EXPECT_TRUE(HI->Type); + EXPECT_FALSE(HI->Value); Oops, this should be FALSE I think. Repository: rG LLVM

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan added a comment. Thank you very much for the ideas. > (generalization) allow partial selection as long as it's of a single node - > the common ancestor is partially selected and no children are This strategy looks reasonable to me and it passes my test cases. I've updated my patch

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan updated this revision to Diff 518277. zyounan added a comment. Do not evaluate on partial selection with children. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148457/new/ https://reviews.llvm.org/D148457 Files:

[PATCH] D148457: [clangd] Support macro evaluation on hover

sammccall added a comment. > Obviously we're expecting evaluation on such macro (which is just what the > original issue addresses). That wasn't (and isn't) obvious to me - the issue didn't mention the macro in question, i assumed it was `#define alignof(x) __alignof(x)` or similar. Looks

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan updated this revision to Diff 518144. zyounan added a comment. Refactor tests. Obtain type from VarDecl. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148457/new/ https://reviews.llvm.org/D148457 Files:

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan added a comment. Haha. My bad for misunderstanding. > I think rather if the common ancestor isn't completely selected, evaluation > shouldn't happen. I don't think it is a feasible approach to simply opt out of partial selection. Please consider this snippet: #define SizeOf sizeof

[PATCH] D148457: [clangd] Support macro evaluation on hover

sammccall added a comment. (This looks really cool, nice work! only a driveby comment from me I'm afraid) In D148457#4304915 , @zyounan wrote: > The dot prior to AST node kind indicates partial selection and the asterisk > indicates complete selection

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan added a comment. Oops, an inline comment disappears accidentally. > #define PLUS_TWO + 2 > int x = 40 PLUS_TW^O; // huh, the value of "+ 2" is "42"? The reason is we're getting a `SelectionNode` with a wider range. Here is the layout of the tree: txt Built selection tree

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan added a comment. Sorry for responding late and thank you for the good catch! I've updated the code and please feel free to leave comments. :) Comment at: clang-tools-extra/clangd/Hover.cpp:728 } + SelectionTree::createEach( + AST.getASTContext(),

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan updated this revision to Diff 517865. zyounan added a comment. Update Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148457/new/ https://reviews.llvm.org/D148457 Files: clang-tools-extra/clangd/Hover.cpp

[PATCH] D148457: [clangd] Support macro evaluation on hover

nridge added a comment. Thanks for the patch and the thorough testcase! I wonder if we should make this a bit more strict: if the macro expansion itself is an expression, show the value of //that// expression, but not e.g. an enclosing expression. Otherwise, the `Definition` field of the hover

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148457/new/ https://reviews.llvm.org/D148457 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D148457: [clangd] Support macro evaluation on hover

zyounan created this revision. Herald added subscribers: kadircet, arphaman. Herald added a project: All. zyounan updated this revision to Diff 513979. zyounan added a comment. zyounan updated this revision to Diff 514019. zyounan updated this revision to Diff 514028. zyounan added reviewers: