[PATCH] D153015: [clangd] Skip function parameter decls when evaluating variables on hover.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

Thanks!




Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3726
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluatedOnInvalidDecls) {
+  struct {

hokein wrote:
> nit: instead of creating a completely-new TEST, it seems simpler to just add 
> a testcase in the existing `TEST(Hover, All)`.
> 
> 
> ```
>{R"cpp(// Should not crash on an invalid param decl.
>   class Foo {};
>   // error-ok
>   void foo(Foo [[fo^o]] = nullptr);
>   )cpp",
>[](HoverInfo ) {
>  HI.Name = "foo";
>  HI.Type = "Foo";
>  HI.Kind = index::SymbolKind::Parameter;
>  HI.NamespaceScope = "";
>  HI.LocalScope = "foo::";
>  HI.Definition = "Foo foo = ";
>}},
> ```
I'm not sure I agree. The `Hover, All` test takes already about 5 seconds to 
scroll through on the screen :)

On top of that, other test cases that are a result of crash fixes seem to be 
separate too.
Therefore, I'd prefer to keep it as a separate test case for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153015

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

[PATCH] D153015: [clangd] Skip function parameter decls when evaluating variables on hover.

[Viktoriia Bakalova via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
VitaNuo marked an inline comment as done.
Closed by commit rGc9888dce4474: [clangd] Skip function parameter decls when 
evaluating variables on hover. (authored by VitaNuo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153015

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
@@ -22,6 +22,7 @@
 
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -3722,6 +3723,34 @@
   EXPECT_EQ(*HI->Value, "");
 }
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluatedOnInvalidDecls) {
+  struct {
+const char *const Code;
+const std::optional HoverValue;
+  } Cases[] = {
+  {R"cpp(
+// error-ok testing behavior on invalid decl
+class Foo {};
+void foo(Foo p^aram = nullptr);
+)cpp",
+   std::nullopt},
+  {R"cpp(
+class Foo {};
+void foo(Foo *p^aram = nullptr);
+)cpp",
+   "nullptr"},
+  };
+
+  for (const auto  : Cases) {
+Annotations T(C.Code);
+TestTU TU = TestTU::withCode(T.code());
+auto AST = TU.build();
+auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+ASSERT_TRUE(HI);
+ASSERT_EQ(HI->Value, C.HoverValue);
+  }
+}
+
 TEST(Hover, DisableShowAKA) {
   Annotations T(R"cpp(
 using m_int = int;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -659,7 +659,7 @@
 HI.Type = printType(TAT->getTemplatedDecl()->getUnderlyingType(), Ctx, PP);
 
   // Fill in value with evaluated initializer if possible.
-  if (const auto *Var = dyn_cast(D)) {
+  if (const auto *Var = dyn_cast(D); Var && !Var->isInvalidDecl()) {
 if (const Expr *Init = Var->getInit())
   HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast(D)) {


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -22,6 +22,7 @@
 
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -3722,6 +3723,34 @@
   EXPECT_EQ(*HI->Value, "");
 }
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluatedOnInvalidDecls) {
+  struct {
+const char *const Code;
+const std::optional HoverValue;
+  } Cases[] = {
+  {R"cpp(
+// error-ok testing behavior on invalid decl
+class Foo {};
+void foo(Foo p^aram = nullptr);
+)cpp",
+   std::nullopt},
+  {R"cpp(
+class Foo {};
+void foo(Foo *p^aram = nullptr);
+)cpp",
+   "nullptr"},
+  };
+
+  for (const auto  : Cases) {
+Annotations T(C.Code);
+TestTU TU = TestTU::withCode(T.code());
+auto AST = TU.build();
+auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+ASSERT_TRUE(HI);
+ASSERT_EQ(HI->Value, C.HoverValue);
+  }
+}
+
 TEST(Hover, DisableShowAKA) {
   Annotations T(R"cpp(
 using m_int = int;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -659,7 +659,7 @@
 HI.Type = printType(TAT->getTemplatedDecl()->getUnderlyingType(), Ctx, PP);
 
   // Fill in value with evaluated initializer if possible.
-  if (const auto *Var = dyn_cast(D)) {
+  if (const auto *Var = dyn_cast(D); Var && !Var->isInvalidDecl()) {
 if (const Expr *Init = Var->getInit())
   HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast(D)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153015: [clangd] Skip function parameter decls when evaluating variables on hover.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Thanks, looks good, just one nit to simplify the unittest.




Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3726
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluatedOnInvalidDecls) {
+  struct {

nit: instead of creating a completely-new TEST, it seems simpler to just add a 
testcase in the existing `TEST(Hover, All)`.


```
   {R"cpp(// Should not crash on an invalid param decl.
  class Foo {};
  // error-ok
  void foo(Foo [[fo^o]] = nullptr);
  )cpp",
   [](HoverInfo ) {
 HI.Name = "foo";
 HI.Type = "Foo";
 HI.Kind = index::SymbolKind::Parameter;
 HI.NamespaceScope = "";
 HI.LocalScope = "foo::";
 HI.Definition = "Foo foo = ";
   }},
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153015

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

[PATCH] D153015: [clangd] Skip function parameter decls when evaluating variables on hover.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:663
+  const auto *Var = dyn_cast(D);
+  if (Var && !llvm::isa(Var)) {
 if (const Expr *Init = Var->getInit())

VitaNuo wrote:
> hokein wrote:
> > We're ignoring all `ParmVarDecl` cases, 
> > 
> > The crash here is for broken code. For the crash case, the AST node looks 
> > like 
> > 
> > ```
> > `-ParmVarDecl 0x563b7cc853c0  col:14 invalid param 
> > 'Foo':'Foo' cinit
> > `-OpaqueValueExpr 0x563b7cc854a0  'Foo':'Foo'
> > ```
> > 
> > One alternative is to exclude invalid `VarDecl`, then the Hover feature 
> > still keep working on valid `ParmVarDecl`.
> > 
> > ```
> > if (const auto* Var = dyn_cast(D); Var && !Var->isInvalidDecl()) {
> >...
> > }
> > ```
> Why would you do that?
Sorry this was an unintended comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153015

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

[PATCH] D153015: [clangd] Skip function parameter decls when evaluating variables on hover.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

Thanks for the review!




Comment at: clang-tools-extra/clangd/Hover.cpp:663
+  const auto *Var = dyn_cast(D);
+  if (Var && !llvm::isa(Var)) {
 if (const Expr *Init = Var->getInit())

hokein wrote:
> We're ignoring all `ParmVarDecl` cases, 
> 
> The crash here is for broken code. For the crash case, the AST node looks 
> like 
> 
> ```
> `-ParmVarDecl 0x563b7cc853c0  col:14 invalid param 
> 'Foo':'Foo' cinit
> `-OpaqueValueExpr 0x563b7cc854a0  'Foo':'Foo'
> ```
> 
> One alternative is to exclude invalid `VarDecl`, then the Hover feature still 
> keep working on valid `ParmVarDecl`.
> 
> ```
> if (const auto* Var = dyn_cast(D); Var && !Var->isInvalidDecl()) {
>...
> }
> ```
Why would you do that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153015

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

[PATCH] D153015: [clangd] Skip function parameter decls when evaluating variables on hover.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo updated this revision to Diff 532044.
VitaNuo marked 2 inline comments as done.
VitaNuo added a comment.

Simplify.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153015

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
@@ -22,6 +22,7 @@
 
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -3722,6 +3723,34 @@
   EXPECT_EQ(*HI->Value, "");
 }
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluatedOnInvalidDecls) {
+  struct {
+const char *const Code;
+const std::optional HoverValue;
+  } Cases[] = {
+  {R"cpp(
+// error-ok testing behavior on invalid decl
+class Foo {};
+void foo(Foo p^aram = nullptr);
+)cpp",
+   std::nullopt},
+  {R"cpp(
+class Foo {};
+void foo(Foo *p^aram = nullptr);
+)cpp",
+   "nullptr"},
+  };
+
+  for (const auto  : Cases) {
+Annotations T(C.Code);
+TestTU TU = TestTU::withCode(T.code());
+auto AST = TU.build();
+auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+ASSERT_TRUE(HI);
+ASSERT_EQ(HI->Value, C.HoverValue);
+  }
+}
+
 TEST(Hover, DisableShowAKA) {
   Annotations T(R"cpp(
 using m_int = int;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -659,7 +659,7 @@
 HI.Type = printType(TAT->getTemplatedDecl()->getUnderlyingType(), Ctx, PP);
 
   // Fill in value with evaluated initializer if possible.
-  if (const auto *Var = dyn_cast(D)) {
+  if (const auto *Var = dyn_cast(D); Var && !Var->isInvalidDecl()) {
 if (const Expr *Init = Var->getInit())
   HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast(D)) {


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -22,6 +22,7 @@
 
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -3722,6 +3723,34 @@
   EXPECT_EQ(*HI->Value, "");
 }
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluatedOnInvalidDecls) {
+  struct {
+const char *const Code;
+const std::optional HoverValue;
+  } Cases[] = {
+  {R"cpp(
+// error-ok testing behavior on invalid decl
+class Foo {};
+void foo(Foo p^aram = nullptr);
+)cpp",
+   std::nullopt},
+  {R"cpp(
+class Foo {};
+void foo(Foo *p^aram = nullptr);
+)cpp",
+   "nullptr"},
+  };
+
+  for (const auto  : Cases) {
+Annotations T(C.Code);
+TestTU TU = TestTU::withCode(T.code());
+auto AST = TU.build();
+auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+ASSERT_TRUE(HI);
+ASSERT_EQ(HI->Value, C.HoverValue);
+  }
+}
+
 TEST(Hover, DisableShowAKA) {
   Annotations T(R"cpp(
 using m_int = int;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -659,7 +659,7 @@
 HI.Type = printType(TAT->getTemplatedDecl()->getUnderlyingType(), Ctx, PP);
 
   // Fill in value with evaluated initializer if possible.
-  if (const auto *Var = dyn_cast(D)) {
+  if (const auto *Var = dyn_cast(D); Var && !Var->isInvalidDecl()) {
 if (const Expr *Init = Var->getInit())
   HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast(D)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153015: [clangd] Skip function parameter decls when evaluating variables on hover.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo updated this revision to Diff 532042.
VitaNuo added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153015

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
@@ -22,6 +22,7 @@
 
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -3722,6 +3723,34 @@
   EXPECT_EQ(*HI->Value, "");
 }
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluatedOnInvalidDecls) {
+  struct {
+const char *const Code;
+const std::optional HoverValue;
+  } Cases[] = {
+  {R"cpp(
+// error-ok testing behavior on invalid decl
+class Foo {};
+void foo(Foo p^aram = nullptr) {}
+)cpp",
+   std::nullopt},
+  {R"cpp(
+class Foo {};
+void foo(Foo *p^aram = nullptr) {}
+)cpp",
+   "nullptr"},
+  };
+
+  for (const auto  : Cases) {
+Annotations T(C.Code);
+TestTU TU = TestTU::withCode(T.code());
+auto AST = TU.build();
+auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+ASSERT_TRUE(HI);
+ASSERT_EQ(HI->Value, C.HoverValue);
+  }
+}
+
 TEST(Hover, DisableShowAKA) {
   Annotations T(R"cpp(
 using m_int = int;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -659,7 +659,7 @@
 HI.Type = printType(TAT->getTemplatedDecl()->getUnderlyingType(), Ctx, PP);
 
   // Fill in value with evaluated initializer if possible.
-  if (const auto *Var = dyn_cast(D)) {
+  if (const auto *Var = dyn_cast(D); Var && !Var->isInvalidDecl()) {
 if (const Expr *Init = Var->getInit())
   HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast(D)) {


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -22,6 +22,7 @@
 
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -3722,6 +3723,34 @@
   EXPECT_EQ(*HI->Value, "");
 }
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluatedOnInvalidDecls) {
+  struct {
+const char *const Code;
+const std::optional HoverValue;
+  } Cases[] = {
+  {R"cpp(
+// error-ok testing behavior on invalid decl
+class Foo {};
+void foo(Foo p^aram = nullptr) {}
+)cpp",
+   std::nullopt},
+  {R"cpp(
+class Foo {};
+void foo(Foo *p^aram = nullptr) {}
+)cpp",
+   "nullptr"},
+  };
+
+  for (const auto  : Cases) {
+Annotations T(C.Code);
+TestTU TU = TestTU::withCode(T.code());
+auto AST = TU.build();
+auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+ASSERT_TRUE(HI);
+ASSERT_EQ(HI->Value, C.HoverValue);
+  }
+}
+
 TEST(Hover, DisableShowAKA) {
   Annotations T(R"cpp(
 using m_int = int;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -659,7 +659,7 @@
 HI.Type = printType(TAT->getTemplatedDecl()->getUnderlyingType(), Ctx, PP);
 
   // Fill in value with evaluated initializer if possible.
-  if (const auto *Var = dyn_cast(D)) {
+  if (const auto *Var = dyn_cast(D); Var && !Var->isInvalidDecl()) {
 if (const Expr *Init = Var->getInit())
   HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast(D)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153015: [clangd] Skip function parameter decls when evaluating variables on hover.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:663
+  const auto *Var = dyn_cast(D);
+  if (Var && !llvm::isa(Var)) {
 if (const Expr *Init = Var->getInit())

We're ignoring all `ParmVarDecl` cases, 

The crash here is for broken code. For the crash case, the AST node looks like 

```
`-ParmVarDecl 0x563b7cc853c0  col:14 invalid param 'Foo':'Foo' 
cinit
`-OpaqueValueExpr 0x563b7cc854a0  'Foo':'Foo'
```

One alternative is to exclude invalid `VarDecl`, then the Hover feature still 
keep working on valid `ParmVarDecl`.

```
if (const auto* Var = dyn_cast(D); Var && !Var->isInvalidDecl()) {
   ...
}
```



Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3726
+TEST(Hover, FunctionParameterDefaulValueNotEvaluated) {
+  Annotations T("void foo(int p^aram = 5);");
+  TestTU TU = TestTU::withCode(T.code());

VitaNuo wrote:
> hokein wrote:
> > I believe this case should trigger a crash, but I don't see the crash with 
> > a trunk-built clangd, do we miss something here?
> It crashes on invalid code. I've inspected two user workspaces:
> 1.
> 
> ```
> class Foo{};
> void foo(Foo param = nullptr);
> ```
> 
> 2.
> ```
> void foo(ClassName param = 
> functionReturningObjectOfSimilarSoundingButUnrelatedClass());
> ```
> 
> I guess `clangd` is not even expected to act soundly in the face of 
> non-compiling code. 
> But since these crashes are easy to get rid of, and evaluating parameters in 
> function declarations is pointless anyways, we can just avoid these crashes 
> at no extra cost. I cannot use non-compiling code in a hover test, though.
thanks for the clarification. That makes sense.

We can still use these broken code (the first one should be good enough) in the 
HoverTest, we need to add  a magic comment `/* error-ok */`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153015

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

[PATCH] D153015: [clangd] Skip function parameter decls when evaluating variables on hover.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

thanks!




Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3726
+TEST(Hover, FunctionParameterDefaulValueNotEvaluated) {
+  Annotations T("void foo(int p^aram = 5);");
+  TestTU TU = TestTU::withCode(T.code());

hokein wrote:
> I believe this case should trigger a crash, but I don't see the crash with a 
> trunk-built clangd, do we miss something here?
It crashes on invalid code. I've inspected two user workspaces:
1.

```
class Foo{};
void foo(Foo param = nullptr);
```

2.
```
void foo(ClassName param = 
functionReturningObjectOfSimilarSoundingButUnrelatedClass());
```

I guess `clangd` is not even expected to act soundly in the face of 
non-compiling code. 
But since these crashes are easy to get rid of, and evaluating parameters in 
function declarations is pointless anyways, we can just avoid these crashes at 
no extra cost. I cannot use non-compiling code in a hover test, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153015

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

[PATCH] D153015: [clangd] Skip function parameter decls when evaluating variables on hover.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

thanks.




Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3726
+TEST(Hover, FunctionParameterDefaulValueNotEvaluated) {
+  Annotations T("void foo(int p^aram = 5);");
+  TestTU TU = TestTU::withCode(T.code());

I believe this case should trigger a crash, but I don't see the crash with a 
trunk-built clangd, do we miss something here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153015

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

[PATCH] D153015: [clangd] Skip function parameter decls when evaluating variables on hover.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo updated this revision to Diff 531718.
VitaNuo added a comment.

Simplify.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153015

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
@@ -3722,6 +3722,15 @@
   EXPECT_EQ(*HI->Value, "");
 }
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluated) {
+  Annotations T("void foo(int p^aram = 5);");
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  ASSERT_FALSE(HI->Value);
+}
+
 TEST(Hover, DisableShowAKA) {
   Annotations T(R"cpp(
 using m_int = int;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -659,7 +659,8 @@
 HI.Type = printType(TAT->getTemplatedDecl()->getUnderlyingType(), Ctx, PP);
 
   // Fill in value with evaluated initializer if possible.
-  if (const auto *Var = dyn_cast(D)) {
+  const auto *Var = dyn_cast(D);
+  if (Var && !llvm::isa(Var)) {
 if (const Expr *Init = Var->getInit())
   HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast(D)) {


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -3722,6 +3722,15 @@
   EXPECT_EQ(*HI->Value, "");
 }
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluated) {
+  Annotations T("void foo(int p^aram = 5);");
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  ASSERT_FALSE(HI->Value);
+}
+
 TEST(Hover, DisableShowAKA) {
   Annotations T(R"cpp(
 using m_int = int;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -659,7 +659,8 @@
 HI.Type = printType(TAT->getTemplatedDecl()->getUnderlyingType(), Ctx, PP);
 
   // Fill in value with evaluated initializer if possible.
-  if (const auto *Var = dyn_cast(D)) {
+  const auto *Var = dyn_cast(D);
+  if (Var && !llvm::isa(Var)) {
 if (const Expr *Init = Var->getInit())
   HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast(D)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153015: [clangd] Skip function parameter decls when evaluating variables on hover.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
VitaNuo requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153015

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
@@ -3722,6 +3722,18 @@
   EXPECT_EQ(*HI->Value, "");
 }
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluated) {
+  Annotations T(R"cpp(
+  void foo(int p^aram = 5);
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  ASSERT_FALSE(HI->Value);
+}
+
 TEST(Hover, DisableShowAKA) {
   Annotations T(R"cpp(
 using m_int = int;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -659,7 +659,8 @@
 HI.Type = printType(TAT->getTemplatedDecl()->getUnderlyingType(), Ctx, PP);
 
   // Fill in value with evaluated initializer if possible.
-  if (const auto *Var = dyn_cast(D)) {
+  const auto *Var = dyn_cast(D);
+  if (Var != nullptr && !llvm::isa(Var)) {
 if (const Expr *Init = Var->getInit())
   HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast(D)) {


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -3722,6 +3722,18 @@
   EXPECT_EQ(*HI->Value, "");
 }
 
+TEST(Hover, FunctionParameterDefaulValueNotEvaluated) {
+  Annotations T(R"cpp(
+  void foo(int p^aram = 5);
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  ASSERT_FALSE(HI->Value);
+}
+
 TEST(Hover, DisableShowAKA) {
   Annotations T(R"cpp(
 using m_int = int;
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -659,7 +659,8 @@
 HI.Type = printType(TAT->getTemplatedDecl()->getUnderlyingType(), Ctx, PP);
 
   // Fill in value with evaluated initializer if possible.
-  if (const auto *Var = dyn_cast(D)) {
+  const auto *Var = dyn_cast(D);
+  if (Var != nullptr && !llvm::isa(Var)) {
 if (const Expr *Init = Var->getInit())
   HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast(D)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits