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

[Nathan Ridge via Phabricator via cfe-commits]

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 (which shows the tokens of the macro expansion) and the `Value` and `Type` fields (which show the value and type of a larger expression) could be out of sync and confusing.

A (somewhat contrived) example would be:

  #define PLUS_TWO + 2
  int x = 40 PLUS_TW^O;  // huh, the value of "+ 2" is "42"?

Of your existing testcases, I think the only one this stricter rule would break is #6 (`define_lambda_begin`), and I think that's fine.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:728
   }
+  SelectionTree::createEach(
+      AST.getASTContext(), AST.getTokens(), SM.getFileOffset(Tok.location()),
----------------
Why `createEach()` when the [non-macro case](https://searchfox.org/llvm/rev/be9c91843bab5bb46574c27836bfcd9ad6fc9ef5/clang-tools-extra/clangd/Hover.cpp#1270) uses `createRight()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148457