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

[Younan Zhang via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D148457

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -18,6 +18,7 @@
 #include "clang/Format/Format.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
 
 #include "gtest/gtest.h"
 #include 
@@ -529,6 +530,8 @@
[](HoverInfo &HI) {
  HI.Name = "MACRO";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Value = "41 (0x29)";
+ HI.Type = "int";
  HI.Definition = "#define MACRO 41\n\n"
  "// Expands to\n"
  "41";
@@ -560,6 +563,7 @@
[](HoverInfo &HI) {
  HI.Name = "DECL_STR";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Type = HoverInfo::PrintedType("const char *");
  HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
  "STRINGIFY(VALUE)\n\n"
  "// Expands to\n"
@@ -1850,6 +1854,8 @@
   )cpp",
   [](HoverInfo &HI) {
 HI.Name = "MACRO";
+HI.Value = "0";
+HI.Type = "int";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0\n\n"
 "// Expands to\n"
@@ -3769,6 +3775,232 @@
   EXPECT_EQ(H->Type->Type, "int");
   EXPECT_EQ(H->Definition, "using foo = type");
 }
+
+TEST(Hover, EvaluateMacros) {
+  llvm::StringRef PredefinedCXX = R"cpp(
+#define X 42
+#define SizeOf sizeof
+#define AlignOf alignof
+#define PLUS_TWO +2
+#define TWO 2
+
+using u64 = unsigned long long;
+// calculate (a ** b) % p
+constexpr u64 pow_with_mod(u64 a, u64 b, u64 p) {
+  u64 ret = 1;
+  while (b) {
+if (b & 1)
+  ret = (ret * a) % p;
+a = (a * a) % p;
+b >>= 1;
+  }
+  return ret;
+}
+#define last_n_digit(x, y, n)  \
+  pow_with_mod(x, y, pow_with_mod(10, n, 2147483647))
+#define declare_struct(X, name, value) \
+  struct X {   \
+constexpr auto name() { return value; }\
+  }
+#define gnu_statement_expression(value)\
+  ({   \
+declare_struct(Widget, getter, value); \
+Widget().getter(); \
+  })
+#define define_lambda_begin(lambda, ...)   \
+  [&](__VA_ARGS__) {
+#define define_lambda_end() }
+
+#define left_bracket [
+#define right_bracket ]
+#define dg_left_bracket <:
+#define dg_right_bracket :>
+#define array_decl(type, name, size) type name left_bracket size right_bracket
+  )cpp";
+
+  struct {
+llvm::StringRef Code;
+const std::function, size_t /*Id*/)>
+Validator;
+  } Cases[] = {
+  {
+  /*Code=*/R"cpp(
+X^;
+  )cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "42 (0x2a)");
+EXPECT_EQ(HI->Type, HoverInfo::PrintedType("int"));
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+Size^Of(int);
+  )cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_TRUE(HI->Value);
+EXPECT_TRUE(HI->Type);
+// Don't validate type or value of `sizeof` and `alignof` as we're
+// getting different values or desugared types on different
+// platforms. Same as below.
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  struct Y {
+int y;
+double z;
+  };
+  Alig^nOf(Y);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_TRUE(HI->Value);
+EXPECT_TRUE(HI->Type);
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  // 2**32 == 4294967296
+  last_n_di^git(2, 32, 6);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "967296 (0xec280)");
+EXPECT_EQ(HI->Type, "u64");
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  gnu_statement_exp^ression(42);
+)cpp",
+  /*Validator=*/
+ 

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

[Younan Zhang via Phabricator via cfe-commits]

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
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -18,6 +18,7 @@
 #include "clang/Format/Format.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
 
 #include "gtest/gtest.h"
 #include 
@@ -529,6 +530,8 @@
[](HoverInfo &HI) {
  HI.Name = "MACRO";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Value = "41 (0x29)";
+ HI.Type = "int";
  HI.Definition = "#define MACRO 41\n\n"
  "// Expands to\n"
  "41";
@@ -560,6 +563,7 @@
[](HoverInfo &HI) {
  HI.Name = "DECL_STR";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Type = HoverInfo::PrintedType("const char *");
  HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
  "STRINGIFY(VALUE)\n\n"
  "// Expands to\n"
@@ -1850,6 +1854,8 @@
   )cpp",
   [](HoverInfo &HI) {
 HI.Name = "MACRO";
+HI.Value = "0";
+HI.Type = "int";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0\n\n"
 "// Expands to\n"
@@ -3769,6 +3775,232 @@
   EXPECT_EQ(H->Type->Type, "int");
   EXPECT_EQ(H->Definition, "using foo = type");
 }
+
+TEST(Hover, EvaluateMacros) {
+  llvm::StringRef PredefinedCXX = R"cpp(
+#define X 42
+#define SizeOf sizeof
+#define AlignOf alignof
+#define PLUS_TWO +2
+#define TWO 2
+
+using u64 = unsigned long long;
+// calculate (a ** b) % p
+constexpr u64 pow_with_mod(u64 a, u64 b, u64 p) {
+  u64 ret = 1;
+  while (b) {
+if (b & 1)
+  ret = (ret * a) % p;
+a = (a * a) % p;
+b >>= 1;
+  }
+  return ret;
+}
+#define last_n_digit(x, y, n)  \
+  pow_with_mod(x, y, pow_with_mod(10, n, 2147483647))
+#define declare_struct(X, name, value) \
+  struct X {   \
+constexpr auto name() { return value; }\
+  }
+#define gnu_statement_expression(value)\
+  ({   \
+declare_struct(Widget, getter, value); \
+Widget().getter(); \
+  })
+#define define_lambda_begin(lambda, ...)   \
+  [&](__VA_ARGS__) {
+#define define_lambda_end() }
+
+#define left_bracket [
+#define right_bracket ]
+#define dg_left_bracket <:
+#define dg_right_bracket :>
+#define array_decl(type, name, size) type name left_bracket size right_bracket
+  )cpp";
+
+  struct {
+llvm::StringRef Code;
+const std::function, size_t /*Id*/)>
+Validator;
+  } Cases[] = {
+  {
+  /*Code=*/R"cpp(
+X^;
+  )cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "42 (0x2a)");
+EXPECT_EQ(HI->Type, HoverInfo::PrintedType("int"));
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+Size^Of(int);
+  )cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_TRUE(HI->Value);
+EXPECT_TRUE(HI->Type);
+// Don't validate type or value of `sizeof` and `alignof` as we're
+// getting different values or desugared types on different
+// platforms. Same as below.
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  struct Y {
+int y;
+double z;
+  };
+  Alig^nOf(Y);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_TRUE(HI->Value);
+EXPECT_TRUE(HI->Type);
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  // 2**32 == 4294967296
+  last_n_di^git(2, 32, 6);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "967296 (0xec280)");
+EXPECT_EQ(HI->Type, "u64");
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  gnu_statement_exp^ression(42);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "42 (0x2a)");
+   

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

[Nathan Ridge via Phabricator via cfe-commits]

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 SelectionTree::Node *N,
+  const ASTContext &Ctx) {

nit: just inline this into its only call site as `doPrintExprValue(N, 
Ctx).PrintedValue` (and then we can call the function `printExprValue` instead 
of `doPrintExprValue`)



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;` where L_BRACKET and

zyounan wrote:
> nridge wrote:
> > This check for punctuators actually makes the behaviour **more strict** for 
> > macros than for non-macros in some cases.
> > 
> > For example:
> > 
> > ```
> > #define PLUS +
> > 
> > constexpr int A = 2 + 3;  // hover over `+` shows `Value = 5`
> > constexpr int B = 2 PLUS 3;  // hover over `PLUS` does not show `Value`
> > ```
> > 
> > I don't think this is a particularly important use case (it's a bit of a 
> > hack to get the expression's value this way, much better would be to select 
> > the entire expression you want to evaluate in the editor, but unfortunately 
> > LSP only sends a single position in `textDocument/hover`, not a full 
> > range...), but perhaps we could consider relaxing this in the future. (I'm 
> > thinking, if we switched to "allow partial selection via macros that expand 
> > to a single token", we could probably drop this condition.)
> Thanks for clarifying this intention (for any bug hunter in the future).
> > if we switched to "allow partial selection via macros that expand to a 
> > single token", we could probably drop this condition.
> 
> I'm afraid we can't. For the array case in the test,
> ```
> vector left_b^racket 3 right_b^racket;
> // left_bracket -> [
> // right_bracket -> ]
> ```
> the associated selection tree is,
> ```
>  TranslationUnitDecl 
>FunctionDecl void function()
>  CompoundStmt { …
>   .ArraySubscriptExpr vector[3]
> ```
> (`function` is the wrapper that facilitates writing single expression, 
> doesn't matter here.)
> It expands to one single token and is partially selected, which exactly 
> matches the strategy. If we do allow evaluation, we'd get a value/type on `[` 
> or `]`. Yes, it is a contrived and weird use case, but for now I think it's 
> fine to keep it unevaluated to avoid issue aforementioned.
> 
> (But I'm willing to remove this restriction if you prefer a relaxed strategy.)
> If we do allow evaluation, we'd get a value/type on [ or ].

Out of curiosity, I tried commenting out this condition locally, and the 
`left_bracket` test case still passed for me.

The reason is that in that test case, the ArraySubscriptExpr cannot be 
evaluated (we don't know what the array values are).

In a modified version of the testcase where the array values are known, but 
macros are not used:

```
int main() {
  constexpr int vector[3] = {1, 2, 3};
  vector [ 2 ];
}
```

hovering over the `[` or `]` does work (in clangd today, no patch needed).

(But maybe a hover in this situation is more confusing than useful, and it 
would be better if it didn't work?)

Anyways, we can discuss this further in follow-up issues like 
https://github.com/clangd/clangd/issues/1622, I think this is good enough for 
now.


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

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

[Younan Zhang via Phabricator via cfe-commits]

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;` where L_BRACKET and

nridge wrote:
> This check for punctuators actually makes the behaviour **more strict** for 
> macros than for non-macros in some cases.
> 
> For example:
> 
> ```
> #define PLUS +
> 
> constexpr int A = 2 + 3;  // hover over `+` shows `Value = 5`
> constexpr int B = 2 PLUS 3;  // hover over `PLUS` does not show `Value`
> ```
> 
> I don't think this is a particularly important use case (it's a bit of a hack 
> to get the expression's value this way, much better would be to select the 
> entire expression you want to evaluate in the editor, but unfortunately LSP 
> only sends a single position in `textDocument/hover`, not a full range...), 
> but perhaps we could consider relaxing this in the future. (I'm thinking, if 
> we switched to "allow partial selection via macros that expand to a single 
> token", we could probably drop this condition.)
Thanks for clarifying this intention (for any bug hunter in the future).
> if we switched to "allow partial selection via macros that expand to a single 
> token", we could probably drop this condition.

I'm afraid we can't. For the array case in the test,
```
vector left_b^racket 3 right_b^racket;
// left_bracket -> [
// right_bracket -> ]
```
the associated selection tree is,
```
 TranslationUnitDecl 
   FunctionDecl void function()
 CompoundStmt { …
  .ArraySubscriptExpr vector[3]
```
(`function` is the wrapper that facilitates writing single expression, doesn't 
matter here.)
It expands to one single token and is partially selected, which exactly matches 
the strategy. If we do allow evaluation, we'd get a value/type on `[` or `]`. 
Yes, it is a contrived and weird use case, but for now I think it's fine to 
keep it unevaluated to avoid issue aforementioned.

(But I'm willing to remove this restriction if you prefer a relaxed strategy.)



Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3832
+  };
+  Alig$3^nOf(Y);
+)cpp",

nridge wrote:
> I guess with this style of test you can simplify the `$3^` to `^`
Aha, that's a typo.


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

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

[Younan Zhang via Phabricator via cfe-commits]

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:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -18,6 +18,7 @@
 #include "clang/Format/Format.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
 
 #include "gtest/gtest.h"
 #include 
@@ -529,6 +530,8 @@
[](HoverInfo &HI) {
  HI.Name = "MACRO";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Value = "41 (0x29)";
+ HI.Type = "int";
  HI.Definition = "#define MACRO 41\n\n"
  "// Expands to\n"
  "41";
@@ -560,6 +563,7 @@
[](HoverInfo &HI) {
  HI.Name = "DECL_STR";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Type = HoverInfo::PrintedType("const char *");
  HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
  "STRINGIFY(VALUE)\n\n"
  "// Expands to\n"
@@ -1792,6 +1796,8 @@
   )cpp",
   [](HoverInfo &HI) {
 HI.Name = "MACRO";
+HI.Value = "0";
+HI.Type = "int";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0\n\n"
 "// Expands to\n"
@@ -3746,6 +3752,232 @@
   EXPECT_EQ(H->Type->Type, "int");
   EXPECT_EQ(H->Definition, "using foo = type");
 }
+
+TEST(Hover, EvaluateMacros) {
+  llvm::StringRef PredefinedCXX = R"cpp(
+#define X 42
+#define SizeOf sizeof
+#define AlignOf alignof
+#define PLUS_TWO +2
+#define TWO 2
+
+using u64 = unsigned long long;
+// calculate (a ** b) % p
+constexpr u64 pow_with_mod(u64 a, u64 b, u64 p) {
+  u64 ret = 1;
+  while (b) {
+if (b & 1)
+  ret = (ret * a) % p;
+a = (a * a) % p;
+b >>= 1;
+  }
+  return ret;
+}
+#define last_n_digit(x, y, n)  \
+  pow_with_mod(x, y, pow_with_mod(10, n, 2147483647))
+#define declare_struct(X, name, value) \
+  struct X {   \
+constexpr auto name() { return value; }\
+  }
+#define gnu_statement_expression(value)\
+  ({   \
+declare_struct(Widget, getter, value); \
+Widget().getter(); \
+  })
+#define define_lambda_begin(lambda, ...)   \
+  [&](__VA_ARGS__) {
+#define define_lambda_end() }
+
+#define left_bracket [
+#define right_bracket ]
+#define dg_left_bracket <:
+#define dg_right_bracket :>
+#define array_decl(type, name, size) type name left_bracket size right_bracket
+  )cpp";
+
+  struct {
+llvm::StringRef Code;
+const std::function, size_t /*Id*/)>
+Validator;
+  } Cases[] = {
+  {
+  /*Code=*/R"cpp(
+X^;
+  )cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "42 (0x2a)");
+EXPECT_EQ(HI->Type, HoverInfo::PrintedType("int"));
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+Size^Of(int);
+  )cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_TRUE(HI->Value);
+EXPECT_TRUE(HI->Type);
+// Don't validate type or value of `sizeof` and `alignof` as we're
+// getting different values or desugared types on different
+// platforms. Same as below.
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  struct Y {
+int y;
+double z;
+  };
+  Alig^nOf(Y);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_TRUE(HI->Value);
+EXPECT_TRUE(HI->Type);
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  // 2**32 == 4294967296
+  last_n_di^git(2, 32, 6);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "967296 (0xec280)");
+EXPECT_EQ(HI->Type, "u64");
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  gnu_statement_exp^ression(42);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+ 

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

[Nathan Ridge via Phabricator via cfe-commits]

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 
> token
> - (generalization) allow partial selection as long as it's of a single node - 
> the common ancestor is partially selected and no children are

I was going to suggest a variant of the second option, where if a macro expands 
to a single token, the behaviour is the same as if the expanded token had been 
written in the source and that's what had been hovered over.

However, I don't have any strong concerns about the additional scenarios 
supported by the third option, so since this is what was implemented, let's go 
with it. We can always tweak this in the future if necessary.

---

Circling back to my original concern:

In D148457#4297847 , @nridge wrote:

> 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

the original selection being "partial" vs. "complete" is just one part of this 
concern. Another part is the loop in `visitExprFromSelectionTree` that can 
choose a larger enclosing expression than the original selection (even if it's 
"complete"), so that we can get a `Definition` for a smaller expression but a 
`Value` for a larger one.

However, I discovered that this sort of discrepancy can already arise without 
macros, where the same loop means `Type` is given for a smaller expression and 
`Value` for a larger one. I filed https://github.com/clangd/clangd/issues/1622 
for this with an example.

Since this is a pre-existing issue, I think it's fine not to worry about it in 
this patch. Perhaps in a follow-up patch we can find a solution that addresses 
both the macro and non-macro cases.




Comment at: clang-tools-extra/clangd/Hover.cpp:471
+const SelectionTree::Node *
+visitExprFromSelectionTree(const SelectionTree::Node *N, const ASTContext &Ctx,
+   llvm::function_ref CB) {

Instead of generalizing `printExprValue` to take an arbitrary callback, can we 
modify the original function to return both the `Expr*` and its printed value 
(grouped in a pair or small struct)? And then our new call site can call 
`getType()` on the `Expr*`.

(I realize this would be a slight change in semantics as the callback for the 
macro codepath currently exits the loop as soon as we have a type... but I 
think there's value in keeping the behaviour consistent between the macro and 
non-macro cases.)



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;` where L_BRACKET and

This check for punctuators actually makes the behaviour **more strict** for 
macros than for non-macros in some cases.

For example:

```
#define PLUS +

constexpr int A = 2 + 3;  // hover over `+` shows `Value = 5`
constexpr int B = 2 PLUS 3;  // hover over `PLUS` does not show `Value`
```

I don't think this is a particularly important use case (it's a bit of a hack 
to get the expression's value this way, much better would be to select the 
entire expression you want to evaluate in the editor, but unfortunately LSP 
only sends a single position in `textDocument/hover`, not a full range...), but 
perhaps we could consider relaxing this in the future. (I'm thinking, if we 
switched to "allow partial selection via macros that expand to a single token", 
we could probably drop this condition.)



Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3832
+  };
+  Alig$3^nOf(Y);
+)cpp",

I guess with this style of test you can simplify the `$3^` to `^`


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

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

[Younan Zhang via Phabricator via cfe-commits]

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

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

[Younan Zhang via Phabricator via cfe-commits]

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:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -18,6 +18,7 @@
 #include "clang/Format/Format.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
 
 #include "gtest/gtest.h"
 #include 
@@ -529,6 +530,8 @@
[](HoverInfo &HI) {
  HI.Name = "MACRO";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Value = "41 (0x29)";
+ HI.Type = "int";
  HI.Definition = "#define MACRO 41\n\n"
  "// Expands to\n"
  "41";
@@ -560,6 +563,7 @@
[](HoverInfo &HI) {
  HI.Name = "DECL_STR";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Type = HoverInfo::PrintedType("const char *");
  HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
  "STRINGIFY(VALUE)\n\n"
  "// Expands to\n"
@@ -1792,6 +1796,8 @@
   )cpp",
   [](HoverInfo &HI) {
 HI.Name = "MACRO";
+HI.Value = "0";
+HI.Type = "int";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0\n\n"
 "// Expands to\n"
@@ -3746,6 +3752,232 @@
   EXPECT_EQ(H->Type->Type, "int");
   EXPECT_EQ(H->Definition, "using foo = type");
 }
+
+TEST(Hover, EvaluateMacros) {
+  llvm::StringRef PredefinedCXX = R"cpp(
+#define X 42
+#define SizeOf sizeof
+#define AlignOf alignof
+#define PLUS_TWO +2
+#define TWO 2
+
+using u64 = unsigned long long;
+// calculate (a ** b) % p
+constexpr u64 pow_with_mod(u64 a, u64 b, u64 p) {
+  u64 ret = 1;
+  while (b) {
+if (b & 1)
+  ret = (ret * a) % p;
+a = (a * a) % p;
+b >>= 1;
+  }
+  return ret;
+}
+#define last_n_digit(x, y, n)  \
+  pow_with_mod(x, y, pow_with_mod(10, n, 2147483647))
+#define declare_struct(X, name, value) \
+  struct X {   \
+constexpr auto name() { return value; }\
+  }
+#define gnu_statement_expression(value)\
+  ({   \
+declare_struct(Widget, getter, value); \
+Widget().getter(); \
+  })
+#define define_lambda_begin(lambda, ...)   \
+  [&](__VA_ARGS__) {
+#define define_lambda_end() }
+
+#define left_bracket [
+#define right_bracket ]
+#define dg_left_bracket <:
+#define dg_right_bracket :>
+#define array_decl(type, name, size) type name left_bracket size right_bracket
+  )cpp";
+
+  struct {
+llvm::StringRef Code;
+const std::function, size_t /*Id*/)>
+Validator;
+  } Cases[] = {
+  {
+  /*Code=*/R"cpp(
+X^;
+  )cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "42 (0x2a)");
+EXPECT_EQ(HI->Type, HoverInfo::PrintedType("int"));
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+Size^Of(int);
+  )cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_TRUE(HI->Value);
+EXPECT_TRUE(HI->Type);
+// Don't validate type or value of `sizeof` and `alignof` as we're
+// getting different values or desugared types on different
+// platforms. Same as below.
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  struct Y {
+int y;
+double z;
+  };
+  Alig$3^nOf(Y);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_TRUE(HI->Value);
+EXPECT_TRUE(HI->Type);
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  // 2**32 == 4294967296
+  last_n_di^git(2, 32, 6);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "967296 (0xec280)");
+EXPECT_EQ(HI->Type, "u64");
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  gnu_statement_exp^ression(42);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+   

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

[Younan Zhang via Phabricator via cfe-commits]

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

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

[Younan Zhang via Phabricator via cfe-commits]

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 again. :)


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

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

[Younan Zhang via Phabricator via cfe-commits]

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:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -18,6 +18,7 @@
 #include "clang/Format/Format.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
 
 #include "gtest/gtest.h"
 #include 
@@ -529,6 +530,8 @@
[](HoverInfo &HI) {
  HI.Name = "MACRO";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Value = "41 (0x29)";
+ HI.Type = "int";
  HI.Definition = "#define MACRO 41\n\n"
  "// Expands to\n"
  "41";
@@ -560,6 +563,7 @@
[](HoverInfo &HI) {
  HI.Name = "DECL_STR";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Type = HoverInfo::PrintedType("const char *");
  HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
  "STRINGIFY(VALUE)\n\n"
  "// Expands to\n"
@@ -1792,6 +1796,8 @@
   )cpp",
   [](HoverInfo &HI) {
 HI.Name = "MACRO";
+HI.Value = "0";
+HI.Type = "int";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0\n\n"
 "// Expands to\n"
@@ -3746,6 +3752,230 @@
   EXPECT_EQ(H->Type->Type, "int");
   EXPECT_EQ(H->Definition, "using foo = type");
 }
+
+TEST(Hover, EvaluateMacros) {
+  llvm::StringRef PredefinedCXX = R"cpp(
+#define X 42
+#define SizeOf sizeof
+#define AlignOf alignof
+#define PLUS_TWO +2
+#define TWO 2
+
+using u64 = unsigned long long;
+// calculate (a ** b) % p
+constexpr u64 pow_with_mod(u64 a, u64 b, u64 p) {
+  u64 ret = 1;
+  while (b) {
+if (b & 1)
+  ret = (ret * a) % p;
+a = (a * a) % p;
+b >>= 1;
+  }
+  return ret;
+}
+#define last_n_digit(x, y, n)  \
+  pow_with_mod(x, y, pow_with_mod(10, n, 2147483647))
+#define declare_struct(X, name, value) \
+  struct X {   \
+constexpr auto name() { return value; }\
+  }
+#define gnu_statement_expression(value)\
+  ({   \
+declare_struct(Widget, getter, value); \
+Widget().getter(); \
+  })
+#define define_lambda_begin(lambda, ...)   \
+  [&](__VA_ARGS__) {
+#define define_lambda_end() }
+
+#define left_bracket [
+#define right_bracket ]
+#define array_decl(type, name, size) type name left_bracket size right_bracket
+  )cpp";
+
+  struct {
+llvm::StringRef Code;
+const std::function, size_t /*Id*/)>
+Validator;
+  } Cases[] = {
+  {
+  /*Code=*/R"cpp(
+X^;
+  )cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "42 (0x2a)");
+EXPECT_EQ(HI->Type, HoverInfo::PrintedType("int"));
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+Size^Of(int);
+  )cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_TRUE(HI->Value);
+EXPECT_TRUE(HI->Type);
+// Don't validate type or value of `sizeof` and `alignof` as we're
+// getting different values or desugared types on different
+// platforms. Same as below.
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  struct Y {
+int y;
+double z;
+  };
+  Alig$3^nOf(Y);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_TRUE(HI->Value);
+EXPECT_TRUE(HI->Type);
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  // 2**32 == 4294967296
+  last_n_di^git(2, 32, 6);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "967296 (0xec280)");
+EXPECT_EQ(HI->Type, "u64");
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+  gnu_statement_exp^ression(42);
+)cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "42 (0x2a)");
+EXPECT_EQ(H

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

[Sam McCall via Phabricator via cfe-commits]

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 
like both forms are in use but clang provides `#define alignof __alignof` as a 
built-in.

A couple of ideas:

- special-case alignof
- (generalization) allow partial selection via macros that expand to a single 
token
- (generalization) allow partial selection as long as it's of a single node - 
the common ancestor is partially selected and no children are


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

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

[Younan Zhang via Phabricator via cfe-commits]

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:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -18,6 +18,7 @@
 #include "clang/Format/Format.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
 
 #include "gtest/gtest.h"
 #include 
@@ -529,6 +530,8 @@
[](HoverInfo &HI) {
  HI.Name = "MACRO";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Value = "41 (0x29)";
+ HI.Type = "int";
  HI.Definition = "#define MACRO 41\n\n"
  "// Expands to\n"
  "41";
@@ -560,6 +563,7 @@
[](HoverInfo &HI) {
  HI.Name = "DECL_STR";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Type = HoverInfo::PrintedType("const char *");
  HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
  "STRINGIFY(VALUE)\n\n"
  "// Expands to\n"
@@ -1792,6 +1796,8 @@
   )cpp",
   [](HoverInfo &HI) {
 HI.Name = "MACRO";
+HI.Value = "0";
+HI.Type = "int";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0\n\n"
 "// Expands to\n"
@@ -3746,6 +3752,213 @@
   EXPECT_EQ(H->Type->Type, "int");
   EXPECT_EQ(H->Definition, "using foo = type");
 }
+
+TEST(Hover, EvaluateMacros) {
+  llvm::StringRef PredefinedCXX = R"cpp(
+#define X 42
+#define SizeOf sizeof
+#define AlignOf alignof
+#define PLUS_TWO +2
+#define TWO 2
+
+using u64 = unsigned long long;
+// calculate (a ** b) % p
+constexpr u64 pow_with_mod(u64 a, u64 b, u64 p) {
+  u64 ret = 1;
+  while (b) {
+if (b & 1)
+  ret = (ret * a) % p;
+a = (a * a) % p;
+b >>= 1;
+  }
+  return ret;
+}
+#define last_n_digit(x, y, n)  \
+  pow_with_mod(x, y, pow_with_mod(10, n, 2147483647))
+#define declare_struct(X, name, value) \
+  struct X {   \
+constexpr auto name() { return value; }\
+  }
+#define gnu_statement_expression(value)\
+  ({   \
+declare_struct(Widget, getter, value); \
+Widget().getter(); \
+  })
+#define define_lambda_begin(lambda, ...)   \
+  [&](__VA_ARGS__) {
+#define define_lambda_end() }
+
+#define left_bracket [
+#define right_bracket ]
+#define array_decl(type, name, size) type name left_bracket size right_bracket
+  )cpp";
+
+  struct {
+llvm::StringRef Code;
+const std::function, size_t /*Id*/)>
+Validator;
+  } Cases[] = {
+  {
+  /*Code=*/R"cpp(
+X^;
+  )cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "42 (0x2a)");
+EXPECT_EQ(HI->Type, HoverInfo::PrintedType("int"));
+  },
+  },
+  {
+  /*Code=*/R"cpp(
+Size^Of(int);
+  )cpp",
+  /*Validator=*/
+  [](std::optional HI, size_t) {
+EXPECT_TRUE(HI->Value);
+EXPECT_TRUE(HI->Type);
+// Don't validate type or value of `sizeof` and `alignof` as we're
+// getting different values or desugared types on different
+// platforms. Same as below.
+  },
+  },
+  {
+  R"cpp(
+  struct Y {
+int y;
+double z;
+  };
+  Alig$3^nOf(Y);
+)cpp",
+  [](std::optional HI, size_t) {
+EXPECT_TRUE(HI->Value);
+EXPECT_TRUE(HI->Type);
+  },
+  },
+  {
+  R"cpp(
+  // 2**32 == 4294967296
+  last_n_di^git(2, 32, 6);
+)cpp",
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "967296 (0xec280)");
+EXPECT_EQ(HI->Type, "u64");
+  },
+  },
+  {
+  R"cpp(
+  gnu_statement_exp^ression(42);
+)cpp",
+  [](std::optional HI, size_t) {
+EXPECT_EQ(HI->Value, "42 (0x2a)");
+EXPECT_EQ(HI->Type, "int");
+  },
+  },
+  {
+  R"cpp(
+  40 + PLU^S_TWO;
+)cpp",
+   

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

[Younan Zhang via Phabricator via cfe-commits]

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
  void check() {
Siz^eOf(int);
  }

And associative selection tree:

  Built selection tree
   TranslationUnitDecl 
 FunctionDecl void check()
   CompoundStmt { …
.UnaryExprOrTypeTraitExpr sizeof(int)  # Partial selected. Exclude it?

Obviously we're expecting evaluation on such macro (which is just what the 
original issue addresses). And it's worth noting that if we define the macro a 
whole expression e.g., `sizeof(int)`, rather than a token `sizeof`, the common 
ancestor will be seen as completely selected.

OTOH, it doesn't seem so trivial to me to tell if an expression or token is 
appropriate (or to say, full enough?) to evaluate. Take Nathan's case for 
example. The macros `#define PLUS_TWO +2` and the `#define PLUS_TWO 2` are both 
transformed into the same AST. The mapped selection tree is,

  .BinaryOperator 40 + 2
 *IntegerLiteral 2  # For either '40 PLU^S_TWO' or '40 + PLU^S_TWO'

...or even more extreme,

  #define PLUS_TWO +2
  40 + PL^US_TWO;  // it should be seen as '+2', right?

As of now I didn't come up with a better idea to split the cases. Any 
suggestion? :)


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

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

[Sam McCall via Phabricator via cfe-commits]

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 according to SelectionTree::print 
> .
>
> By invoking `Tree.commonAncestor()` it would stop at `BinaryOperator` despite 
> the fact that we're expecting `IntegerLiteral`, that represents the number 
> being accordance with the macro expansion.
>
> In order to address such case, I think we could go down the `SelectionTree` a 
> little further and start evaluating at the node with complete selection.

I think rather if the common ancestor isn't completely selected, evaluation 
shouldn't happen. (I think this is what Nathan was suggesting by "strict"?)

(Here, "+ 2" doesn't have a value, the + is binary rather than unary)

The partial/fully selected concept in SelectionTree was originally intended for 
exactly this kind of thing, so I'm happy to see it get some use :-)


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

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

[Younan Zhang via Phabricator via cfe-commits]

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
   TranslationUnitDecl 
 FunctionDecl void check()
   CompoundStmt { …
.BinaryOperator 40 + 2
  *IntegerLiteral 2

The dot prior to AST node kind indicates partial selection and the asterisk 
indicates complete selection according to SelectionTree::print 
.

By invoking `Tree.commonAncestor()` it would stop at `BinaryOperator` despite 
the fact that we're expecting `IntegerLiteral`, that represents the number 
being accordance with the macro expansion.

In order to address such case, I think we could go down the `SelectionTree` a 
little further and start evaluating at the node with complete selection.


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

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

[Younan Zhang via Phabricator via cfe-commits]

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(), AST.getTokens(), SM.getFileOffset(Tok.location()),

nridge wrote:
> Why `createEach()` when the [non-macro 
> case](https://searchfox.org/llvm/rev/be9c91843bab5bb46574c27836bfcd9ad6fc9ef5/clang-tools-extra/clangd/Hover.cpp#1270)
>  uses `createRight()`?
TBH, I didn't notice that we're using `createRight()` before and I'm not quite 
sure if there exists such case, that yields nothing with the right selection 
tree (which is exactly the 1st tree produced by `createEach()`) but could be 
evaluated with falling back to the left selection tree.
I'll change it to `createEach` anyway.


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

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

[Younan Zhang via Phabricator via cfe-commits]

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
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -529,6 +529,8 @@
[](HoverInfo &HI) {
  HI.Name = "MACRO";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Value = "41 (0x29)";
+ HI.Type = "int";
  HI.Definition = "#define MACRO 41\n\n"
  "// Expands to\n"
  "41";
@@ -1792,6 +1794,8 @@
   )cpp",
   [](HoverInfo &HI) {
 HI.Name = "MACRO";
+HI.Value = "0";
+HI.Type = "int";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0\n\n"
 "// Expands to\n"
@@ -3746,6 +3750,140 @@
   EXPECT_EQ(H->Type->Type, "int");
   EXPECT_EQ(H->Definition, "using foo = type");
 }
+
+TEST(Hover, EvaluateMacros) {
+  Annotations CXX(R"cpp(
+  #define X 42
+  #define SizeOf sizeof
+  #define AlignOf alignof
+
+  using u64 = unsigned long long;
+  // calculate (a ** b) % p
+  constexpr u64 pow_with_mod(u64 a, u64 b, u64 p) {
+u64 ret = 1;
+while (b) {
+  if (b & 1)
+ret = (ret * a) % p;
+  a = (a * a) % p;
+  b >>= 1;
+}
+return ret;
+  }
+  #define last_n_digit(x, y, n)  \
+pow_with_mod(x, y, pow_with_mod(10, n, 2147483647))
+  #define declare_struct(X, name, value) \
+struct X {   \
+  constexpr auto name() { return value; }\
+}
+  #define gnu_statement_expression(value)\
+({   \
+  declare_struct(Widget, getter, value); \
+  Widget().getter(); \
+})
+  #define define_lambda_begin(lambda, ...)   \
+[&](__VA_ARGS__) {
+  #define define_lambda_end() }
+
+  #define plus_42 +40
+
+void check() {
+  X$1^;
+  Size$2^Of(int);
+  struct Y {
+int y;
+double z;
+  };
+  Alig$3^nOf(Y);
+  // 2**32 == 4294967296
+  last_n_di$4^git(2, 32, 6);
+  gnu_statement_exp$5^ression(42);
+
+  constexpr auto value = define_lamb$6^da_begin(lambda, int, char)
+// Check if the expansion range is right.
+return $7^last_n_digit(10, 3, 3)$8^;
+  define_lam$9^bda_end();
+  2 pl$10^us_42;
+}
+  )cpp");
+
+  Config Cfg;
+  Cfg.Hover.ShowAKA = false;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+  auto TU = TestTU::withCode(CXX.code());
+  TU.ExtraArgs.push_back("-std=c++17");
+  auto GetHoverAt = [AST(TU.build()), CXX](llvm::StringRef Point) mutable {
+return getHover(AST, CXX.point(Point), format::getLLVMStyle(), nullptr);
+  };
+
+  std::optional H;
+
+  H = GetHoverAt("1");
+  ASSERT_TRUE(H);
+  EXPECT_EQ(H->Value, "42 (0x2a)");
+  EXPECT_EQ(H->Type, HoverInfo::PrintedType("int"));
+
+  H = GetHoverAt("2");
+  ASSERT_TRUE(H);
+  EXPECT_EQ(H->Value, "4");
+  // Don't validate type of `sizeof` and `alignof` as we're getting different
+  // desugared types on different platforms. Same as below.
+
+  H = GetHoverAt("3");
+  ASSERT_TRUE(H);
+  EXPECT_EQ(H->Value, "8");
+
+  H = GetHoverAt("4");
+  ASSERT_TRUE(H);
+  EXPECT_EQ(H->Value, "967296 (0xec280)");
+  EXPECT_EQ(H->Type, HoverInfo::PrintedType("u64"));
+
+  H = GetHoverAt("5");
+  ASSERT_TRUE(H);
+  EXPECT_EQ(H->Value, "42 (0x2a)");
+  EXPECT_EQ(H->Type, HoverInfo::PrintedType("int"));
+
+  H = GetHoverAt("6");
+  ASSERT_TRUE(H);
+  EXPECT_FALSE(H->Value) << H->Value;
+  EXPECT_EQ(H->Type, HoverInfo::PrintedType("class (lambda)")) << H->Type;
+
+  H = GetHoverAt("7");
+  ASSERT_TRUE(H);
+  EXPECT_EQ(H->Value, "0");
+  EXPECT_EQ(H->Type, HoverInfo::PrintedType("u64"));
+
+  H = GetHoverAt("8");
+  ASSERT_FALSE(H);
+
+  H = GetHoverAt("9");
+  ASSERT_FALSE(H->Value) << H->Value;
+  ASSERT_FALSE(H->Type) << H->Type;
+
+  H = GetHoverAt("10");
+  ASSERT_TRUE(H);
+  EXPECT_EQ(H->Type, HoverInfo::PrintedType("int"));
+  EXPECT_EQ(H->Value, "40 (0x28)");
+
+  Annotations C(R"c(
+#define alignof _Alignof
+void foo() {
+  al^ignof(struct { int x; char y[10]; });
+}
+  )c");
+
+  TU = TestTU::withCode(C.code());
+  TU.Filename = "TestTU.c";
+  TU.ExtraArgs = {
+  "-std=c17",
+  };
+  auto AST = TU.build(

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Younan Zhang via Phabricator via cfe-commits]

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

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

[Younan Zhang via Phabricator via cfe-commits]

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: nridge, sammccall, kadircet.
zyounan published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

~~Fix tests on Windows~~


zyounan added a comment.

unsigned long isn't 8 bytes on windows


zyounan added a comment.

Do not validate type of builtin operators


Creating a SelectionTree at the location where macro expands allows
us to obtain the associated expression, which might then be used to
evaluate compile-time values if possible.

Closes clangd/clangd#1595.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148457

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -529,6 +529,8 @@
[](HoverInfo &HI) {
  HI.Name = "MACRO";
  HI.Kind = index::SymbolKind::Macro;
+ HI.Value = "41 (0x29)";
+ HI.Type = "int";
  HI.Definition = "#define MACRO 41\n\n"
  "// Expands to\n"
  "41";
@@ -1792,6 +1794,8 @@
   )cpp",
   [](HoverInfo &HI) {
 HI.Name = "MACRO";
+HI.Value = "0";
+HI.Type = "int";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0\n\n"
 "// Expands to\n"
@@ -3746,6 +3750,132 @@
   EXPECT_EQ(H->Type->Type, "int");
   EXPECT_EQ(H->Definition, "using foo = type");
 }
+
+TEST(Hover, EvaluateMacros) {
+  Annotations CXX(R"cpp(
+  #define X 42
+  #define SizeOf sizeof
+  #define AlignOf alignof
+
+  using u64 = unsigned long long;
+  // calculate (a ** b) % p
+  constexpr u64 pow_with_mod(u64 a, u64 b, u64 p) {
+u64 ret = 1;
+while (b) {
+  if (b & 1)
+ret = (ret * a) % p;
+  a = (a * a) % p;
+  b >>= 1;
+}
+return ret;
+  }
+  #define last_n_digit(x, y, n)  \
+pow_with_mod(x, y, pow_with_mod(10, n, 2147483647))
+  #define declare_struct(X, name, value) \
+struct X {   \
+  constexpr auto name() { return value; }\
+}
+  #define gnu_statement_expression(value)\
+({   \
+  declare_struct(Widget, getter, value); \
+  Widget().getter(); \
+})
+  #define define_lambda_begin(lambda, ...)   \
+[&](__VA_ARGS__) {
+  #define define_lambda_end() }
+
+void check() {
+  X$1^;
+  Size$2^Of(int);
+  struct Y {
+int y;
+double z;
+  };
+  Alig$3^nOf(Y);
+  // 2**32 == 4294967296
+  last_n_di$4^git(2, 32, 6);
+  gnu_statement_exp$5^ression(42);
+
+  constexpr auto value = define_lamb$6^da_begin(lambda, int, char)
+// Check if the expansion range is right.
+return $7^last_n_digit(10, 3, 3)$8^;
+  define_lam$9^bda_end();
+}
+  )cpp");
+
+  Config Cfg;
+  Cfg.Hover.ShowAKA = false;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+  auto TU = TestTU::withCode(CXX.code());
+  TU.ExtraArgs.push_back("-std=c++17");
+  auto GetHoverAt = [AST(TU.build()), CXX](llvm::StringRef Point) mutable {
+return getHover(AST, CXX.point(Point), format::getLLVMStyle(), nullptr);
+  };
+
+  std::optional H;
+
+  H = GetHoverAt("1");
+  ASSERT_TRUE(H);
+  EXPECT_EQ(H->Value, "42 (0x2a)");
+  EXPECT_EQ(H->Type, HoverInfo::PrintedType("int"));
+
+  H = GetHoverAt("2");
+  ASSERT_TRUE(H);
+  EXPECT_EQ(H->Value, "4");
+  // Don't validate type of `sizeof` and `alignof` as we're getting different
+  // desugared types on different platforms. Same as below.
+
+  H = GetHoverAt("3");
+  ASSERT_TRUE(H);
+  EXPECT_EQ(H->Value, "8");
+
+  H = GetHoverAt("4");
+  ASSERT_TRUE(H);
+  EXPECT_EQ(H->Value, "967296 (0xec280)");
+  EXPECT_EQ(H->Type, HoverInfo::PrintedType("u64"));
+
+  H = GetHoverAt("5");
+  ASSERT_TRUE(H);
+  EXPECT_EQ(H->Value, "42 (0x2a)");
+  EXPECT_EQ(H->Type, HoverInfo::PrintedType("int"));
+
+  H = GetHoverAt("6");
+  ASSERT_TRUE(H);
+  EXPECT_FALSE(H->Value) << H->Value;
+  EXPECT_EQ(H->Type, HoverInfo::PrintedType("class (lambda)")) << H->Type;
+
+  H = GetHoverAt("7");
+  ASSE