[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Thank you! Since the documentation shouldn't be available until LLVM 17, I'm 
slowly working on it. Let me update the PR in this weekend :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

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

[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added inline comments.



Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1343
 #undef CMDMACRO
 $inactive3[[#ifdef CMDMACRO
   int inactiveInt2;

hokein wrote:
> While this patch is an improvement, I wonder we should move it further.
> 
> Has been thinking about it more, we seem to have some inconsistent behavior 
> on highlighting the `#if`, `#else`, `#endif` lines:
> 
> - in `$inactive1` case, the `#endif` is marked as inactive (IMO, the 
> highlighting in the editor is confusing, it looks like we're missing a match 
> `endif`, thus I prefer to mark it as active to correspond to the active `#if` 
> branch);
> - in `$inactive3` case, the line `#elif PREAMBLEMACRO > 0` is marked as 
> inactive, this is inconsistent with `$inactive1` case;
> 
> I think it would be nice to have a consistent model. One approach is to 
> always consider `#if`, `#elif`, `#endif`, `#endif` lines active (in the 
> implementation side, this would mean we always use the line range 
> [skipp_range.start.line+1, skipp_range.end.line - 1]).
> 
> What do you think about this?
> 
> 
+1. My perspective is that C++ source code is actually a meta-language 
(preprocessor) that describes generation of C++ language code. That is, `#if`, 
`#else`, `#endif` and .etc are always in a sense "executed" to generate the 
actual code. So they shouldn't be marked as inactive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151190

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

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz marked an inline comment as done.
daiyousei-qz added a comment.

Addressed the remaining comments. Thanks everyone for helping review and 
improve this patch!




Comment at: clang-tools-extra/clangd/InlayHints.cpp:802
+Position HintStart = sourceLocToPosition(SM, RBraceLoc);
+Position HintEnd = {HintStart.line,
+HintStart.character +

sammccall wrote:
> I'd prefer `sourceLocToPosition(SM, 
> RBraceLoc.getLocationWithOffset(HintRangeText.size()))` which would avoids 
> such low-level conversion between coordinate systems, and seems to perform 
> just fine (we're going to hit SourceManager's caches).
> 
> Will leave this up to you though.
Yeah, this is definitely clearer. Updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

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

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 525922.
daiyousei-qz marked 8 inline comments as done.
daiyousei-qz added a comment.

- Address remaining comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -77,6 +77,7 @@
   C.InlayHints.Parameters = false;
   C.InlayHints.DeducedTypes = false;
   C.InlayHints.Designators = false;
+  C.InlayHints.BlockEnd = false;
   return C;
 }
 
@@ -122,6 +123,15 @@
   assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
 }
 
+template 
+void assertBlockEndHints(llvm::StringRef AnnotatedSource,
+ ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.BlockEnd = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::BlockEnd, AnnotatedSource, Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -1550,6 +1560,194 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(BlockEndHints, Functions) {
+  assertBlockEndHints(R"cpp(
+int foo() {
+  return 41;
+$foo[[}]]
+
+template 
+int bar() { 
+  // No hint for lambda for now
+  auto f = []() { 
+return X; 
+  };
+  return f(); 
+$bar[[}]]
+
+// No hint because this isn't a definition
+int buz();
+
+struct S{};
+bool operator==(S, S) {
+  return true;
+$opEqual[[}]]
+  )cpp",
+  ExpectedHint{" // foo", "foo"},
+  ExpectedHint{" // bar", "bar"},
+  ExpectedHint{" // operator==", "opEqual"});
+}
+
+TEST(BlockEndHints, Methods) {
+  assertBlockEndHints(R"cpp(
+struct Test {
+  // No hint because there's no function body
+  Test() = default;
+  
+  ~Test() {
+  $dtor[[}]]
+
+  void method1() {
+  $method1[[}]]
+
+  // No hint because this isn't a definition
+  void method2();
+
+  template 
+  void method3() {
+  $method3[[}]]
+
+  // No hint because this isn't a definition
+  template 
+  void method4();
+
+  Test operator+(int) const {
+return *this;
+  $opIdentity[[}]]
+
+  operator bool() const {
+return true;
+  $opBool[[}]]
+
+  // No hint because there's no function body
+  operator int() const = delete;
+} x;
+
+void Test::method2() {
+$method2[[}]]
+
+template 
+void Test::method4() {
+$method4[[}]]
+  )cpp",
+  ExpectedHint{" // ~Test", "dtor"},
+  ExpectedHint{" // method1", "method1"},
+  ExpectedHint{" // method3", "method3"},
+  ExpectedHint{" // operator+", "opIdentity"},
+  ExpectedHint{" // operator bool", "opBool"},
+  ExpectedHint{" // Test::method2", "method2"},
+  ExpectedHint{" // Test::method4", "method4"});
+}
+
+TEST(BlockEndHints, Namespaces) {
+  assertBlockEndHints(
+  R"cpp(
+namespace {
+  void foo();
+$anon[[}]]
+
+namespace ns {
+  void bar();
+$ns[[}]]
+  )cpp",
+  ExpectedHint{" // namespace", "anon"},
+  ExpectedHint{" // namespace ns", "ns"});
+}
+
+TEST(BlockEndHints, Types) {
+  assertBlockEndHints(
+  R"cpp(
+struct S {
+$S[[};]]
+
+class C {
+$C[[};]]
+
+union U {
+$U[[};]]
+
+enum E1 {
+$E1[[};]]
+
+enum class E2 {
+$E2[[};]]
+  )cpp",
+  ExpectedHint{" // struct S", "S"}, ExpectedHint{" // class C", "C"},
+  ExpectedHint{" // union U", "U"}, ExpectedHint{" // enum E1", "E1"},
+  ExpectedHint{" // enum class E2", "E2"});
+}
+
+TEST(BlockEndHints, TrailingSemicolon) {
+  assertBlockEndHints(R"cpp(
+// The hint is placed after the trailing ';'
+struct S1 {
+$S1[[}  ;]]   
+
+// The hint is always placed in the same line with the closing '}'.
+// So in this case where ';' is missing, it is attached to '}'.
+struct S2 {
+$S2[[}]]
+
+;
+
+// No hint because only one trailing ';' is allowed
+struct S3 {
+};;
+
+// No hint because trailing ';' is only allowed for class/struct/union/enum
+void foo() {
+};
+
+// Rare case, but yes we'll have a hint here.
+struct {
+

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 525448.
daiyousei-qz added a comment.

- Fix reversed logic of including inactive region token


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "Annotations.h"
+#include "Config.h"
 #include "Protocol.h"
 #include "SemanticHighlighting.h"
 #include "SourceCode.h"
@@ -1260,6 +1261,17 @@
   EXPECT_EQ(Toks[3].deltaStart, 2u);
   EXPECT_EQ(Toks[3].length, 3u);
 }
+
+TEST(SemanticHighlighting, WithHighlightingFilter) {
+  llvm::StringRef AnnotatedCode = R"cpp(
+int *$Variable[[x]] = new int;
+)cpp";
+  Config Cfg;
+  Cfg.SemanticTokens.DisabledKinds = {"Operator"};
+  Cfg.SemanticTokens.DisabledModifiers = {"Declaration", "Definition"};
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  checkHighlightings(AnnotatedCode, {}, ~ScopeModifierMask);
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -246,6 +246,23 @@
   EXPECT_EQ(Results[0].InlayHints.DeducedTypes, std::nullopt);
 }
 
+TEST(ParseYAML, SemanticTokens) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+SemanticTokens:
+  DisabledKinds: [ Operator, InactiveCode]
+  DisabledModifiers: Readonly
+  )yaml");
+  auto Results =
+  Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledKinds,
+  ElementsAre(val("Operator"), val("InactiveCode")));
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledModifiers,
+  ElementsAre(val("Readonly")));
+}
+
 TEST(ParseYAML, IncludesIgnoreHeader) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -61,6 +61,8 @@
 };
 
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingKind K);
+std::optional
+highlightingKindFromString(llvm::StringRef Name);
 
 enum class HighlightingModifier {
   Declaration,
@@ -88,6 +90,8 @@
 static_assert(static_cast(HighlightingModifier::LastModifier) < 32,
   "Increase width of modifiers bitfield!");
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingModifier K);
+std::optional
+highlightingModifierFromString(llvm::StringRef Name);
 
 // Contains all information needed for the highlighting a token.
 struct HighlightingToken {
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "SemanticHighlighting.h"
+#include "Config.h"
 #include "FindTarget.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
@@ -354,13 +355,57 @@
   return Winner;
 }
 
+/// Filter to remove particular kinds of highlighting tokens and modifiers from
+/// the output.
+class HighlightingFilter {
+public:
+  HighlightingFilter() {
+for (auto  : ActiveKindLookup)
+  Active = true;
+
+ActiveModifiersMask = ~0;
+  }
+
+  void disableKind(HighlightingKind Kind) {
+ActiveKindLookup[static_cast(Kind)] = false;
+  }
+
+  void disableModifier(HighlightingModifier Modifier) {
+ActiveModifiersMask &= ~(1 << static_cast(Modifier));
+  }
+
+  bool isHighlightKindActive(HighlightingKind Kind) const {
+return ActiveKindLookup[static_cast(Kind)];
+  }
+
+  uint32_t maskModifiers(uint32_t Modifiers) const {
+return Modifiers & ActiveModifiersMask;
+  }
+
+  static HighlightingFilter fromCurrentConfig() {
+const Config  = Config::current();
+HighlightingFilter Filter;
+for (const auto  : C.SemanticTokens.DisabledKinds)
+  if 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:578
+constexpr unsigned HintMinLineLimit = 2;
+constexpr unsigned HintMaxLengthLimit = 50;
+

nridge wrote:
> daiyousei-qz wrote:
> > sammccall wrote:
> > > We actually already have a configurable inlay hint length limit.
> > > It has the wrong name (`TypeNameLimit`) , but I think we should use it 
> > > anyway (and maybe try to fix the name later).
> > > 
> > > (I'm not sure making this configurable was a great idea, but given that 
> > > it exists, people will expect it to effectively limit the length of hints)
> > `TypeNameLimit` was introduced to limit the length of "DeducedTypes" only. 
> > I don't think using that number for all hints is a good idea because 
> > different hints appear in different contexts. For deduced type hint, it is 
> > displayed in the middle of the actual code, so it must be concise to not 
> > overwhelm the actual code. However, this hint is usually displayed at the 
> > end of an almost empty line. A much larger number of characters should be 
> > allowed.
> > 
> > (I'm not arguing here, but IMO such options are definitely helpful. Not 
> > everybody would fork llvm-project and build their own version of clangd 
> > binary. Without options to configure length of hints, people would disable 
> > "DeducedTypes" entirely if they cannot tolerate the default length limit, 
> > while this feature is definitely a cool thing to turn on. Personally, I 
> > actually think clangd has too little option compared to say rust-analyzer. 
> > But it's just my understanding)
> > For deduced type hint, it is displayed in the middle of the actual code, so 
> > it must be concise to not overwhelm the actual code. However, this hint is 
> > usually displayed at the end of an almost empty line. A much larger number 
> > of characters should be allowed.
> 
> Another consideration besides the location of the hint that is worth keeping 
> in mind, is what type of content is being printed.
> 
> Type names in C++ are composable in ways that identifiers are not (e.g. 
> `vector, allocator, 
> allocator>` etc.), such that there is a need to limit type 
> hints that doesn't really apply to say, parameter hints.
> 
> So, a question I have here is: can template argument lists appear in an 
> end-definition-comment hint?
> 
> For example, for:
> 
> ```
> template 
> struct S {
>   void foo();
> };
> 
> template 
> void S::foo() {
> }  // <--- HERE
> ```
> 
> is the hint at the indicated location `S::foo()`, or `S::foo()`? In the 
> latter case, we can imagine the hint getting long due to the type parameters, 
> and we may want to consider either limiting its length, or tweaking the code 
> to not print the type parameters.
Yes, currently this hint is only shown if the total length of hint label is 
less than 60 characters. I believe what we display should be exactly what is 
used to define the symbol. In your example,

```
template 
struct S {
  void foo();
};

template 
void S::foo() {
}  // S::foo
```
Basically, "[A]" and "[B]" is the same text (OFC excluding whitespaces).
```
void [A]() {
} // [B]
```
The hint won't be displayed if "[B]" is more than 60 characters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

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

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Sorry for the inactivity. I have replace the parameter with const ref. By the 
way, I also removed the existing `IncludeInactiveRegionTokens` flag and uses 
this filter instead. Please help review the change. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

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

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 525421.
daiyousei-qz added a comment.

- Address review comment and remove a unnecessary flag


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "Annotations.h"
+#include "Config.h"
 #include "Protocol.h"
 #include "SemanticHighlighting.h"
 #include "SourceCode.h"
@@ -1260,6 +1261,17 @@
   EXPECT_EQ(Toks[3].deltaStart, 2u);
   EXPECT_EQ(Toks[3].length, 3u);
 }
+
+TEST(SemanticHighlighting, WithHighlightingFilter) {
+  llvm::StringRef AnnotatedCode = R"cpp(
+int *$Variable[[x]] = new int;
+)cpp";
+  Config Cfg;
+  Cfg.SemanticTokens.DisabledKinds = {"Operator"};
+  Cfg.SemanticTokens.DisabledModifiers = {"Declaration", "Definition"};
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  checkHighlightings(AnnotatedCode, {}, ~ScopeModifierMask);
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -246,6 +246,23 @@
   EXPECT_EQ(Results[0].InlayHints.DeducedTypes, std::nullopt);
 }
 
+TEST(ParseYAML, SemanticTokens) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+SemanticTokens:
+  DisabledKinds: [ Operator, InactiveCode]
+  DisabledModifiers: Readonly
+  )yaml");
+  auto Results =
+  Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledKinds,
+  ElementsAre(val("Operator"), val("InactiveCode")));
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledModifiers,
+  ElementsAre(val("Readonly")));
+}
+
 TEST(ParseYAML, IncludesIgnoreHeader) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -61,6 +61,8 @@
 };
 
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingKind K);
+std::optional
+highlightingKindFromString(llvm::StringRef Name);
 
 enum class HighlightingModifier {
   Declaration,
@@ -88,6 +90,8 @@
 static_assert(static_cast(HighlightingModifier::LastModifier) < 32,
   "Increase width of modifiers bitfield!");
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingModifier K);
+std::optional
+highlightingModifierFromString(llvm::StringRef Name);
 
 // Contains all information needed for the highlighting a token.
 struct HighlightingToken {
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "SemanticHighlighting.h"
+#include "Config.h"
 #include "FindTarget.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
@@ -354,13 +355,57 @@
   return Winner;
 }
 
+/// Filter to remove particular kinds of highlighting tokens and modifiers from
+/// the output.
+class HighlightingFilter {
+public:
+  HighlightingFilter() {
+for (auto  : ActiveKindLookup)
+  Active = true;
+
+ActiveModifiersMask = ~0;
+  }
+
+  void disableKind(HighlightingKind Kind) {
+ActiveKindLookup[static_cast(Kind)] = false;
+  }
+
+  void disableModifier(HighlightingModifier Modifier) {
+ActiveModifiersMask &= ~(1 << static_cast(Modifier));
+  }
+
+  bool isHighlightKindActive(HighlightingKind Kind) const {
+return ActiveKindLookup[static_cast(Kind)];
+  }
+
+  uint32_t maskModifiers(uint32_t Modifiers) const {
+return Modifiers & ActiveModifiersMask;
+  }
+
+  static HighlightingFilter fromCurrentConfig() {
+const Config  = Config::current();
+HighlightingFilter Filter;
+for (const auto  : C.SemanticTokens.DisabledKinds)
+  if 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Addressed review comments except for those we don't have an agreement yet.




Comment at: clang-tools-extra/clangd/InlayHints.cpp:578
+constexpr unsigned HintMinLineLimit = 2;
+constexpr unsigned HintMaxLengthLimit = 50;
+

sammccall wrote:
> We actually already have a configurable inlay hint length limit.
> It has the wrong name (`TypeNameLimit`) , but I think we should use it anyway 
> (and maybe try to fix the name later).
> 
> (I'm not sure making this configurable was a great idea, but given that it 
> exists, people will expect it to effectively limit the length of hints)
`TypeNameLimit` was introduced to limit the length of "DeducedTypes" only. I 
don't think using that number for all hints is a good idea because different 
hints appear in different contexts. For deduced type hint, it is displayed in 
the middle of the actual code, so it must be concise to not overwhelm the 
actual code. However, this hint is usually displayed at the end of an almost 
empty line. A much larger number of characters should be allowed.

(I'm not arguing here, but IMO such options are definitely helpful. Not 
everybody would fork llvm-project and build their own version of clangd binary. 
Without options to configure length of hints, people would disable 
"DeducedTypes" entirely if they cannot tolerate the default length limit, while 
this feature is definitely a cool thing to turn on. Personally, I actually 
think clangd has too little option compared to say rust-analyzer. But it's just 
my understanding)



Comment at: clang-tools-extra/clangd/InlayHints.cpp:597
+auto BlockBeginLine =
+SM.getSpellingLineNumber(BraceRange.getBegin(), );
+if (Invalid)

sammccall wrote:
> this should be getFileLoc(BraceRange.getBegin()), I think?
Thanks for pointing out! As per comments below, calling `getLineNumber` instead 
of `getSpellingLineNumber` in the latest version.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:783
+// Note this range doesn't include the trailing ';' in type definitions.
+// So we have to add SkippedChars to the end character.
+SourceRange R = D.getSourceRange();

sammccall wrote:
> daiyousei-qz wrote:
> > sammccall wrote:
> > > This is too much arithmetic and fiddly coupling between this function and 
> > > shouldHintEndDefinitionComment.
> > > 
> > > Among other things, it allows for confusion between unicode characters 
> > > (UTF-32), clang bytes (UTF-8), and LSP characters (usually UTF-16). And 
> > > we do have a bug here: shouldHintEndDefinition provides SkippedChars in 
> > > clang bytes, but you add it to end.character below which is in LSP 
> > > characters.
> > > 
> > > ---
> > > 
> > > Let's redesign a little... 
> > > 
> > > We have a `}` on some line. We want to compute a sensible part of that 
> > > line to attach to.
> > > A suitable range may not exist, in which case we're going to omit the 
> > > hint.
> > > 
> > > - The line consists of text we don't care about , the `}`, and then some 
> > > mix of whitespace, "trivial" punctuation, and "nontrivial" chars.
> > > - the range should always start at the `}`, since that's what we're 
> > > really hinting
> > > - to get the hint in the right place, the range should end after the 
> > > trivial chars, but before any trailing whitespace
> > > - if there are any nontrivial chars, there's no suitable range
> > > 
> > > so something like:
> > > 
> > > ```
> > > optional findBraceTargetRange(SourceLocation CloseBrace) {
> > >   auto [File, Offset] = SM.getDecomposedLoc(SM.getFileLoc(CloseBrace));
> > >   if (File != MainFileID) return std::nullopt;
> > >   StringRef RestOfLine = 
> > > MainFileBuf.substr(Offset).split('\n').first.rtrim();
> > >   if (!RestOfLine.consume_front("{")) return;
> > >   if (StringRef::npos != Punctuation.find_first_of(" ,;")) return 
> > > std::nullopt;
> > >   return {offsetToPosition(MainFileBuf, Offset), 
> > > offsetToPosition(MainFileBuf, Result.bytes_end() - 
> > > MainFileBuf.bytes_end()};
> > > }
> > > ```
> > > 
> > > and then call from addEndDefinitionComment, the result is LSPRange 
> > > already.
> > Done, also moved min-line and max-length logic to this function. Btw, I 
> > think we should avoid `offsetToPosition` as much as possible. It is a large 
> > overhead considering inlay hints are recomputed throughout the entire file 
> > for each edit. I frequently work with source code that's nearly 1MB large 
> > (Yes, I don't think we should have source file this large, but it is what 
> > it is).
> > also moved min-line and max-length logic to this function
> 
> I'd prefer you to move them back. As specified, that function is otherwise 
> strictly about examining the textual source code around the closing brace. 
> Now it's muddled: it also looks at the opening brace, and does lookups into 
> line tables.
> 
> You seem to be aiming to optimize 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 525418.
daiyousei-qz marked 9 inline comments as done.
daiyousei-qz added a comment.

- Move computeBlockEndHintRange
- Correct label length limit logic
- Correct line number computation for '{'
- Address other review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -77,6 +77,7 @@
   C.InlayHints.Parameters = false;
   C.InlayHints.DeducedTypes = false;
   C.InlayHints.Designators = false;
+  C.InlayHints.BlockEnd = false;
   return C;
 }
 
@@ -122,6 +123,15 @@
   assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
 }
 
+template 
+void assertBlockEndHints(llvm::StringRef AnnotatedSource,
+ ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.BlockEnd = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::BlockEnd, AnnotatedSource, Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -1550,6 +1560,194 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(BlockEndHints, Functions) {
+  assertBlockEndHints(R"cpp(
+int foo() {
+  return 41;
+$foo[[}]]
+
+template 
+int bar() { 
+  // No hint for lambda for now
+  auto f = []() { 
+return X; 
+  };
+  return f(); 
+$bar[[}]]
+
+// No hint because this isn't a definition
+int buz();
+
+struct S{};
+bool operator==(S, S) {
+  return true;
+$opEqual[[}]]
+  )cpp",
+  ExpectedHint{" // foo", "foo"},
+  ExpectedHint{" // bar", "bar"},
+  ExpectedHint{" // operator==", "opEqual"});
+}
+
+TEST(BlockEndHints, Methods) {
+  assertBlockEndHints(R"cpp(
+struct Test {
+  // No hint because there's no function body
+  Test() = default;
+  
+  ~Test() {
+  $dtor[[}]]
+
+  void method1() {
+  $method1[[}]]
+
+  // No hint because this isn't a definition
+  void method2();
+
+  template 
+  void method3() {
+  $method3[[}]]
+
+  // No hint because this isn't a definition
+  template 
+  void method4();
+
+  Test operator+(int) const {
+return *this;
+  $opIdentity[[}]]
+
+  operator bool() const {
+return true;
+  $opBool[[}]]
+
+  // No hint because there's no function body
+  operator int() const = delete;
+} x;
+
+void Test::method2() {
+$method2[[}]]
+
+template 
+void Test::method4() {
+$method4[[}]]
+  )cpp",
+  ExpectedHint{" // ~Test", "dtor"},
+  ExpectedHint{" // method1", "method1"},
+  ExpectedHint{" // method3", "method3"},
+  ExpectedHint{" // operator+", "opIdentity"},
+  ExpectedHint{" // operator bool", "opBool"},
+  ExpectedHint{" // Test::method2", "method2"},
+  ExpectedHint{" // Test::method4", "method4"});
+}
+
+TEST(BlockEndHints, Namespaces) {
+  assertBlockEndHints(
+  R"cpp(
+namespace {
+  void foo();
+$anon[[}]]
+
+namespace ns {
+  void bar();
+$ns[[}]]
+  )cpp",
+  ExpectedHint{" // namespace", "anon"},
+  ExpectedHint{" // namespace ns", "ns"});
+}
+
+TEST(BlockEndHints, Types) {
+  assertBlockEndHints(
+  R"cpp(
+struct S {
+$S[[};]]
+
+class C {
+$C[[};]]
+
+union U {
+$U[[};]]
+
+enum E1 {
+$E1[[};]]
+
+enum class E2 {
+$E2[[};]]
+  )cpp",
+  ExpectedHint{" // struct S", "S"}, ExpectedHint{" // class C", "C"},
+  ExpectedHint{" // union U", "U"}, ExpectedHint{" // enum E1", "E1"},
+  ExpectedHint{" // enum class E2", "E2"});
+}
+
+TEST(BlockEndHints, TrailingSemicolon) {
+  assertBlockEndHints(R"cpp(
+// The hint is placed after the trailing ';'
+struct S1 {
+$S1[[}  ;]]   
+
+// The hint is always placed in the same line with the closing '}'.
+// So in this case where ';' is missing, it is attached to '}'.
+struct S2 {
+$S2[[}]]
+
+;
+
+// No hint because only one trailing ';' is allowed
+struct S3 {
+};;
+
+// No hint because trailing ';' is only allowed for 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Yeah, makes sense. Thank you for your insight! I missed the fact that this time 
include AST build time.

I manually injected a time region into `inlayHints`, here's part of logs 
printed for scrolling "offsetToPosition" build:

  I[23:36:43.055] <-- textDocument/hover(21)
  I[23:36:43.057] --> reply:textDocument/hover(21) 1 ms
  I[23:36:43.057] --> textDocument/clangd.fileStatus
  I[23:36:44.422] <-- textDocument/inlayHint(22)
  E[23:36:44.612] ++ Inlay hints takes 189 ms to compute
  I[23:36:44.612] --> reply:textDocument/inlayHint(22) 189 ms
  I[23:36:44.612] --> textDocument/clangd.fileStatus
  I[23:36:44.875] <-- textDocument/inlayHint(23)
  E[23:36:45.053] ++ Inlay hints takes 178 ms to compute
  I[23:36:45.054] --> reply:textDocument/inlayHint(23) 179 ms
  I[23:36:45.054] --> textDocument/clangd.fileStatus
  I[23:36:45.686] <-- textDocument/inlayHint(24)
  E[23:36:45.865] ++ Inlay hints takes 178 ms to compute
  I[23:36:45.865] --> reply:textDocument/inlayHint(24) 179 ms
  I[23:36:45.865] --> textDocument/clangd.fileStatus

And here's logs scrolling "sourceLocToPosition" + "lspLength":

  I[23:41:04.695] <-- textDocument/inlayHint(73)
  I[23:41:04.707] --> reply:textDocument/inlayHint(73) 12 ms
  I[23:41:04.707] --> textDocument/clangd.fileStatus
  I[23:41:04.793] <-- textDocument/inlayHint(74)
  I[23:41:04.807] --> reply:textDocument/inlayHint(74) 13 ms
  I[23:41:04.807] --> textDocument/clangd.fileStatus
  I[23:41:04.913] <-- textDocument/inlayHint(75)
  I[23:41:04.926] --> reply:textDocument/inlayHint(75) 12 ms
  I[23:41:04.926] --> textDocument/clangd.fileStatus
  I[23:41:05.407] <-- textDocument/inlayHint(76)
  I[23:41:05.420] --> reply:textDocument/inlayHint(76) 12 ms
  I[23:41:05.420] --> textDocument/clangd.fileStatus

As this is more than 10x slower, I'm holding my position that we shouldn't use 
`offsetToPosition` for LSP features triggered per edit/scroll. 150ms is more 
than noticeable.

If you want to repro on your side, replace computation of HintStart/HintEnd 
with the following code

  Position HintStart = offsetToPosition(MainFileBuf, RBraceOffset);
  Position HintEnd =
  offsetToPosition(MainFileBuf, RBraceOffset + HintRangeText.size());

Btw, it's always fascinating to see how modern computer system is so fast, and 
in the meantime still so "underpowered" :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

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

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

I'm not sure but 18ms is definitely wrong here, especially for a debug build. 
In your log, even semantic highlighting builds in 60ms, which looks incorrect. 
I tried again with TOT clangd and noticed the following pattern.

- If I scroll with vscode, inlay hints typically computes in 10~20ms
- If I edit the source code, inlay hints computes in around 600ms

I'm not sure why that's happening. Maybe @nridge could have a better answer. 
Somehow, this patch seems introduce a significant slowdown. I'll investigate 
further to see why that's happening.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

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

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:616
+Position HintStart = sourceLocToPosition(SM, BraceRange.getEnd());
+Position HintEnd = {HintStart.line,
+HintStart.character +

sammccall wrote:
> please don't do this, call sourceLocToPosition unless you have benchmarks 
> showing this is a bottleneck on real-world code.
> 
> `lspLength` and direct manipulation of line/character is unneccesarily subtle.
> (If the performance matters, there's no need to be computing the LSP line of 
> the lbrace at all - we never use it - this is one reason I think this 
> function has the wrong signature)
I argue performance does matter here. I eye-balled inlay hint compute time in 
clangd's output window from vscode. On "./clang/lib/Sema/SemaOpenMP.cpp" which 
is around 1MB, using `sourceLocToPosition` roughly takes around 1250~1350ms. 
However, using two `offsetLocToPosition` usually use 1400~1500ms.  I think 
100ms delta is already a noticeable difference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

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

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

In D148489#4362990 , @nridge wrote:

> (Do you plan to address the last comment before I merge?)

Oh, okay. Let me address that. Didn't see that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

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

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Yes please.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

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

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz marked an inline comment as done.
daiyousei-qz added a comment.

Addressed most review comments.

I'm currently using the name "BlockEnd" as suggested. Though I'm proposing a 
name like "DeclBlockEnd" to make it clearer. What do you think?




Comment at: clang-tools-extra/clangd/InlayHints.cpp:285-286
+  bool VisitEnumDecl(EnumDecl *D) {
+if (Cfg.InlayHints.EndDefinitionComments &&
+D->isThisDeclarationADefinition()) {
+  StringRef DeclPrefix;

zyounan wrote:
> nit: bail out early?
> 
No longer needed as merged into VisitTagDecl



Comment at: clang-tools-extra/clangd/InlayHints.cpp:777
+  void addEndDefinitionCommentHint(const NamedDecl , StringRef DeclPrefix,
+   bool SkipSemicolon) {
+size_t SkippedChars = 0;

sammccall wrote:
> SkipSemicolon doesn't need to be optional, a trailing semicolon never adds 
> any ambiguity about what the hint is for
Yes, no ambiguity. But the following hint looks weird to me:
```
void foo() {
}; // foo
```

Since this isn't that complicated to filter them out, I'd prefer making it more 
precise.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:783
+// Note this range doesn't include the trailing ';' in type definitions.
+// So we have to add SkippedChars to the end character.
+SourceRange R = D.getSourceRange();

sammccall wrote:
> This is too much arithmetic and fiddly coupling between this function and 
> shouldHintEndDefinitionComment.
> 
> Among other things, it allows for confusion between unicode characters 
> (UTF-32), clang bytes (UTF-8), and LSP characters (usually UTF-16). And we do 
> have a bug here: shouldHintEndDefinition provides SkippedChars in clang 
> bytes, but you add it to end.character below which is in LSP characters.
> 
> ---
> 
> Let's redesign a little... 
> 
> We have a `}` on some line. We want to compute a sensible part of that line 
> to attach to.
> A suitable range may not exist, in which case we're going to omit the hint.
> 
> - The line consists of text we don't care about , the `}`, and then some mix 
> of whitespace, "trivial" punctuation, and "nontrivial" chars.
> - the range should always start at the `}`, since that's what we're really 
> hinting
> - to get the hint in the right place, the range should end after the trivial 
> chars, but before any trailing whitespace
> - if there are any nontrivial chars, there's no suitable range
> 
> so something like:
> 
> ```
> optional findBraceTargetRange(SourceLocation CloseBrace) {
>   auto [File, Offset] = SM.getDecomposedLoc(SM.getFileLoc(CloseBrace));
>   if (File != MainFileID) return std::nullopt;
>   StringRef RestOfLine = MainFileBuf.substr(Offset).split('\n').first.rtrim();
>   if (!RestOfLine.consume_front("{")) return;
>   if (StringRef::npos != Punctuation.find_first_of(" ,;")) return 
> std::nullopt;
>   return {offsetToPosition(MainFileBuf, Offset), 
> offsetToPosition(MainFileBuf, Result.bytes_end() - MainFileBuf.bytes_end()};
> }
> ```
> 
> and then call from addEndDefinitionComment, the result is LSPRange already.
Done, also moved min-line and max-length logic to this function. Btw, I think 
we should avoid `offsetToPosition` as much as possible. It is a large overhead 
considering inlay hints are recomputed throughout the entire file for each 
edit. I frequently work with source code that's nearly 1MB large (Yes, I don't 
think we should have source file this large, but it is what it is).



Comment at: clang-tools-extra/clangd/InlayHints.cpp:797
+  // differently.
+  assert(Label.empty());
+  Label += printName(AST, D);

zyounan wrote:
> nit: `assert(Label.empty() && "Label should be empty with FunctionDecl")`. 
> might be helpful for debugging.
No longer needed as this assert is removed.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:671
+
+  void addInlayHint(Range LSPRange, HintSide Side, InlayHintKind Kind,
+llvm::StringRef Prefix, llvm::StringRef Label,

sammccall wrote:
> I don't really like this signature change.
> 
> I understand the goal to avoid duplicating the range computation but it seems 
> unlikely to be critical.
> Also, the caller could get the line numbers of the `{}` from the 
> SourceManager and compare those, which should be cheaper than computing the 
> range, so we wouldn't be duplicating all of the work.
As per another comment below, this change is kept.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:760
+// Note this range doesn't include the trailing ';' in type definitions.
+SourceRange R = D.getSourceRange();
+auto LSPRange = getHintRange(R);

sammccall wrote:
> I believe it makes more sense to target the `}` rather than the whole 
> declaration here - we're really talking about what the `}` closes, rather 
> than 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 524183.
daiyousei-qz marked 19 inline comments as done.
daiyousei-qz added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -77,6 +77,7 @@
   C.InlayHints.Parameters = false;
   C.InlayHints.DeducedTypes = false;
   C.InlayHints.Designators = false;
+  C.InlayHints.BlockEnd = false;
   return C;
 }
 
@@ -122,6 +123,15 @@
   assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
 }
 
+template 
+void assertBlockEndHints(llvm::StringRef AnnotatedSource,
+ ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.BlockEnd = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::BlockEnd, AnnotatedSource, Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -1550,6 +1560,194 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(BlockEndHints, Functions) {
+  assertBlockEndHints(R"cpp(
+int foo() {
+  return 41;
+$foo[[}]]
+
+template 
+int bar() { 
+  // No hint for lambda for now
+  auto f = []() { 
+return X; 
+  };
+  return f(); 
+$bar[[}]]
+
+// No hint because this isn't a definition
+int buz();
+
+struct S{};
+bool operator==(S, S) {
+  return true;
+$opEqual[[}]]
+  )cpp",
+  ExpectedHint{" // foo", "foo"},
+  ExpectedHint{" // bar", "bar"},
+  ExpectedHint{" // operator==", "opEqual"});
+}
+
+TEST(BlockEndHints, Methods) {
+  assertBlockEndHints(R"cpp(
+struct Test {
+  // No hint because there's no function body
+  Test() = default;
+  
+  ~Test() {
+  $dtor[[}]]
+
+  void method1() {
+  $method1[[}]]
+
+  // No hint because this isn't a definition
+  void method2();
+
+  template 
+  void method3() {
+  $method3[[}]]
+
+  // No hint because this isn't a definition
+  template 
+  void method4();
+
+  Test operator+(int) const {
+return *this;
+  $opIdentity[[}]]
+
+  operator bool() const {
+return true;
+  $opBool[[}]]
+
+  // No hint because there's no function body
+  operator int() const = delete;
+} x;
+
+void Test::method2() {
+$method2[[}]]
+
+template 
+void Test::method4() {
+$method4[[}]]
+  )cpp",
+  ExpectedHint{" // ~Test", "dtor"},
+  ExpectedHint{" // method1", "method1"},
+  ExpectedHint{" // method3", "method3"},
+  ExpectedHint{" // operator+", "opIdentity"},
+  ExpectedHint{" // operator bool", "opBool"},
+  ExpectedHint{" // Test::method2", "method2"},
+  ExpectedHint{" // Test::method4", "method4"});
+}
+
+TEST(BlockEndHints, Namespaces) {
+  assertBlockEndHints(
+  R"cpp(
+namespace {
+  void foo();
+$anon[[}]]
+
+namespace ns {
+  void bar();
+$ns[[}]]
+  )cpp",
+  ExpectedHint{" // namespace", "anon"},
+  ExpectedHint{" // namespace ns", "ns"});
+}
+
+TEST(BlockEndHints, Types) {
+  assertBlockEndHints(
+  R"cpp(
+struct S {
+$S[[};]]
+
+class C {
+$C[[};]]
+
+union U {
+$U[[};]]
+
+enum E1 {
+$E1[[};]]
+
+enum class E2 {
+$E2[[};]]
+  )cpp",
+  ExpectedHint{" // struct S", "S"}, ExpectedHint{" // class C", "C"},
+  ExpectedHint{" // union U", "U"}, ExpectedHint{" // enum E1", "E1"},
+  ExpectedHint{" // enum class E2", "E2"});
+}
+
+TEST(BlockEndHints, TrailingSemicolon) {
+  assertBlockEndHints(R"cpp(
+// The hint is placed after the trailing ';'
+struct S1 {
+$S1[[}  ;]]   
+
+// The hint is always placed in the same line with the closing '}'.
+// So in this case where ';' is missing, it is attached to '}'.
+struct S2 {
+$S2[[}]]
+
+;
+
+// No hint because only one trailing ';' is allowed
+struct S3 {
+};;
+
+// No hint because trailing ';' is only allowed for class/struct/union/enum
+void foo() {
+};
+
+// Rare case, but yes we'll have a hint here.
+struct {
+  

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

For a quick peek to the hint:

F27483170: image.png 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

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

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

@zyounan np! Feel free to leave your review comments!

@sammccall Thank you for you insightful and detailed review! I addressed many 
review comments in the updated patch and left things I'm not very sure.

Regarding to **Naming**, I agree that `EndDefinitionComment` sounds so tedious 
but I couldn't come up with a better name. However, I think `EndBlock` also 
feel a little strange to me as a public name. If I come across a setting key in 
`InlayHints` section named `EndBlock`, honestly I would be confused by what it 
would do ("end" could be a verb, and "block" usually make me to think of the 
local scope block). What about `AfterBlock` or `AfterDefinition`?

Regarding to **Configuration**, I suppose the line of definition is a very 
natural parameter for this feature. I doubt we could come up with some good 
heuristic to conditional show this hint. Considering how straightforward this 
parameter its implementation is, I'd prefer to having this around if possible. 
But I'll let you to decide.




Comment at: clang-tools-extra/clangd/InlayHints.cpp:763
+if (!LSPRange ||
+static_cast(LSPRange->end.line - LSPRange->start.line) + 1 <
+Cfg.InlayHints.EndDefinitionCommentMinLines)

sammccall wrote:
> I'm not sure we're using the right range here. Consider:
> 
> ```
> const some::long* function
>many params, therefore spanning, lines) {}
> ```
> 
> I don't think hinting `}` here is useful: the relevant distance is from `{` 
> rather than the start of the declaration.
I agree. What do you think is a good way get the range of the definition block? 
What I can think of is to walking backwards the expanded token buffer from the 
ending '}' and searching for the balanced '}'.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:771
+if (const auto *FD = dyn_cast_or_null()) {
+  Label = printName(AST, D);
+} else {

sammccall wrote:
> why is printName OK here and not below?
I'm using printName here to print names for operator overload. Do you have any 
advice?



Comment at: clang-tools-extra/clangd/InlayHints.cpp:777-781
+  else if (isa(D)) {
+Label += "enum ";
+if (cast(D).isScopedUsingClassTag()) {
+  Label += "class ";
+}

sammccall wrote:
> zyounan wrote:
> > Perhaps duplicate the logic from [[ 
> > https://clang.llvm.org/doxygen/DeclPrinter_8cpp_source.html#l00529 | 
> > DeclPrinter::VisitEnumDecl ]]? For `enum struct`, printing `enum` only 
> > might confuse users.
> > 
> I'm torn here: "enum class Foo" is wordy so "enum Foo" is tempting to me. 
> Don't have a strong opinion.
> 
> regardless, printing the tag for class but not struct is definitely wrong :-)
Fixed `enum struct`. Honestly, just learnt there's such a thing, lol.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1566
+
+TEST(EndDefinitionHints, Functions) {
+  assertEndDefinitionHints(R"cpp(

sammccall wrote:
> can you add testcases where:
>  - the function name is an operator
>  - the function is templated
Added template function. I don't quite understand what you mean by "the 
function name is an operator". I'm assuming you meant operator overload so 
closing this one as covered below.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1576
+  )cpp",
+   0, ExpectedHint{" /* foo */ ", "foo"},
+   ExpectedHint{" /* bar */ ", "bar"});

sammccall wrote:
> we're not adding the kind here as in "function foo".
> 
> Maybe that's OK, "function" is long, doesn't appear in the C++ syntax, and is 
> mostly obvious.
> But in that case I'd like to have some hint, maybe `/* foo() */`? (Not the 
> actual param list of course, the parens would always be empty)
> 
> If you don't think we need any hint, then I'd question whether we need 
> "class" etc for class hints!
I think just `// foo` is good enough. Having no "qualifier" could be a 
privilege for functions since they are usually the majority of the source code. 
When reading through the source code, people will get that if no prefix is 
attached, then it's a function. (OFC they'll also see `// namespace` and .etc, 
but rare)

Honestly, IMO `// foo()` is worse than just the name. It could mislead me to 
think that this may be a complete signature while it is definitely not.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1580
+
+TEST(EndDefinitionHints, Methods) {
+  assertEndDefinitionHints(R"cpp(

sammccall wrote:
> can you add a test with a lambda?
> I think the hint should probably just be `lambda` or `(lambda)`, this 
> probably needs special handling.
> 
> It definitely shouldn't be `operator()` or `(lambda at somefile.cc:47)`.
I would argue we don't need hint for a lambda. This hint is only added to 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 522898.
daiyousei-qz marked 16 inline comments as done.
daiyousei-qz added a comment.

- addressed many review comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -77,6 +77,7 @@
   C.InlayHints.Parameters = false;
   C.InlayHints.DeducedTypes = false;
   C.InlayHints.Designators = false;
+  C.InlayHints.EndDefinitionComments = false;
   return C;
 }
 
@@ -122,6 +123,17 @@
   assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
 }
 
+template 
+void assertEndDefinitionHints(llvm::StringRef AnnotatedSource,
+  uint32_t MinLines, ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.EndDefinitionComments = true;
+  Cfg.InlayHints.EndDefinitionCommentMinLines = MinLines;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::EndDefinitionComment, AnnotatedSource,
+  Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -1550,6 +1562,196 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(EndDefinitionHints, Functions) {
+  assertEndDefinitionHints(R"cpp(
+$foo[[int foo() {
+  return 41;
+}]]
+
+template 
+$bar[[int bar() { return X; }]]
+
+// No hint because this isn't a definition
+int buz();
+  )cpp",
+   0, ExpectedHint{" // foo", "foo"},
+   ExpectedHint{" // bar", "bar"});
+}
+
+TEST(EndDefinitionHints, Methods) {
+  assertEndDefinitionHints(R"cpp(
+class Test {
+public:
+  // No hint because there's no function body
+  Test() = default;
+  
+  $dtor[[~Test() {}]]
+
+  $method1[[void method1() {}]]
+
+  // No hint because this isn't a definition
+  void method2();
+
+  template 
+  $method3[[void method3() {}]]
+
+  // No hint because this isn't a definition
+  template 
+  void method4();
+} x;
+
+$method2[[void Test::method2() {}]]
+
+template 
+$method4[[void Test::method4() {}]]
+  )cpp",
+   0, ExpectedHint{" // ~Test", "dtor"},
+   ExpectedHint{" // method1", "method1"},
+   ExpectedHint{" // method3", "method3"},
+   ExpectedHint{" // Test::method2", "method2"},
+   ExpectedHint{" // Test::method4", "method4"});
+}
+
+TEST(EndDefinitionHints, OverloadedOperators) {
+  assertEndDefinitionHints(R"cpp(
+struct S {
+  $opId[[S operator+(int) const {
+return *this;
+  }]]
+
+  $opBool[[operator bool() const {
+return true;
+  }]]
+
+  // No hint because there's no function body
+  operator int() const = delete;
+
+  // No hint because this isn't a definition
+  operator float() const;
+} x;
+
+$opEq[[bool operator==(const S&, const S&) {
+  return true;
+}]]
+  )cpp",
+   0, ExpectedHint{" // operator+", "opId"},
+   ExpectedHint{" // operator bool", "opBool"},
+   ExpectedHint{" // operator==", "opEq"});
+}
+
+TEST(EndDefinitionHints, Namespaces) {
+  assertEndDefinitionHints(
+  R"cpp(
+$anon[[namespace {
+  void foo();
+}]]
+
+$ns[[namespace ns {
+  void bar();
+}]]
+  )cpp",
+  0, ExpectedHint{" // namespace", "anon"},
+  ExpectedHint{" // namespace ns", "ns"});
+}
+
+TEST(EndDefinitionHints, Types) {
+  assertEndDefinitionHints(
+  R"cpp(
+$S[[struct S {
+};]]
+
+$C[[class C {
+};]]
+
+$U[[union U {
+};]]
+
+$E1[[enum E1 {
+};]]
+
+$E2[[enum class E2 {
+};]]
+  )cpp",
+  0, ExpectedHint{" // struct S", "S"}, ExpectedHint{" // class C", "C"},
+  ExpectedHint{" // union U", "U"}, ExpectedHint{" // enum E1", "E1"},
+  ExpectedHint{" // enum class E2", "E2"});
+}
+
+TEST(EndDefinitionHints, BundledTypeVariableDecl) {
+  assertEndDefinitionHints(
+  R"cpp(
+// No hint because we have a declarator right after '}'
+struct {
+  int x;
+} s;
+
+// Rare case, but yes we'll have a hint here.
+ 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Requesting for some advice




Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1666
+
+$anon[[struct {
+  int x;

This is unwanted behavior from my understanding. Do you guys have any insight 
how we could fix this? What I can think of is tracking if we are currently 
inside a variable declaration and turning off the hint. However, this would 
have some side effects on
```
auto f = [] {
   struct S {};
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

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

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 522434.
daiyousei-qz added a comment.

minor naming fix2 (last fix breaks builds)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -77,6 +77,7 @@
   C.InlayHints.Parameters = false;
   C.InlayHints.DeducedTypes = false;
   C.InlayHints.Designators = false;
+  C.InlayHints.EndDefinitionComments = false;
   return C;
 }
 
@@ -122,6 +123,17 @@
   assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
 }
 
+template 
+void assertEndDefinitionHints(llvm::StringRef AnnotatedSource,
+  uint32_t MinLines, ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.EndDefinitionComments = true;
+  Cfg.InlayHints.EndDefinitionCommentMinLines = MinLines;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::EndDefinitionComment, AnnotatedSource,
+  Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -1550,6 +1562,165 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(EndDefinitionHints, Functions) {
+  assertEndDefinitionHints(R"cpp(
+$foo[[int foo() {
+  return 41;
+}]]
+$bar[[int bar() {}]]
+
+// No hint because this isn't a definition
+int buz();
+  )cpp",
+   0, ExpectedHint{" /* foo */ ", "foo"},
+   ExpectedHint{" /* bar */ ", "bar"});
+}
+
+TEST(EndDefinitionHints, Methods) {
+  assertEndDefinitionHints(R"cpp(
+class Test {
+public:
+  $ctor[[Test() = default]];
+  $dtor[[~Test() {
+  }]]
+
+  $method[[void method() {}]]
+
+  // No hint because this isn't a definition
+  void method2();
+} x;
+  )cpp",
+   0, ExpectedHint{" /* Test */ ", "ctor"},
+   ExpectedHint{" /* ~Test */ ", "dtor"},
+   ExpectedHint{" /* method */ ", "method"});
+}
+
+TEST(EndDefinitionHints, OverloadedOperators) {
+  assertEndDefinitionHints(R"cpp(
+struct S {
+  $opAdd[[S operator+(int) const {
+return *this;
+  }]]
+
+  $opBool[[operator bool() const {
+return true;
+  }]]
+
+  $opInt[[operator int() const = delete]];
+
+  // No hint because this isn't a definition
+  operator float() const;
+} x;
+  )cpp",
+   0, ExpectedHint{" /* operator+ */ ", "opAdd"},
+   ExpectedHint{" /* operator bool */ ", "opBool"},
+   ExpectedHint{" /* operator int */ ", "opInt"});
+}
+
+TEST(EndDefinitionHints, Namespaces) {
+  assertEndDefinitionHints(
+  R"cpp(
+$anon[[namespace {
+}]]
+
+$ns[[namespace ns {
+  void foo();
+}]]
+  )cpp",
+  0, ExpectedHint{" /* namespace  */ ", "anon"},
+  ExpectedHint{" /* namespace ns */ ", "ns"});
+}
+
+TEST(EndDefinitionHints, Types) {
+  assertEndDefinitionHints(R"cpp(
+$S[[struct S {
+}]];
+
+$C[[class C {
+}]];
+
+$U[[union U {
+}]];
+
+$E1[[enum E1 {
+}]];
+
+$E2[[enum class E2 {
+}]];
+  )cpp",
+   0, ExpectedHint{" /* struct S */ ", "S"},
+   ExpectedHint{" /* class C */ ", "C"},
+   ExpectedHint{" /* union U */ ", "U"},
+   ExpectedHint{" /* enum E1 */ ", "E1"},
+   ExpectedHint{" /* enum class E2 */ ", "E2"});
+}
+
+TEST(EndDefinitionHints, BundledTypeVariableDecl) {
+  assertEndDefinitionHints(
+  R"cpp(
+struct {
+  int x;
+} s;
+
+$anon[[struct {
+  int x;
+}]]
+
+s2;
+  )cpp",
+  0, ExpectedHint{" /* struct  */ ", "anon"});
+}
+
+TEST(EndDefinitionHints, TrailingSemicolon) {
+  assertEndDefinitionHints(R"cpp(
+$S1[[struct S1 {
+}]];
+
+$S2[[struct S2 {
+}]]
+
+;
+
+$S3[[struct S3 {
+}]] ;; ;;
+  )cpp",
+   0, ExpectedHint{" /* struct S1 */ ", "S1"},
+   ExpectedHint{" /* struct S2 */ ", "S2"},
+   ExpectedHint{" /* struct S3 */ ", "S3"});
+}
+
+TEST(EndDefinitionHints, TrailingText) {
+  

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 522432.
daiyousei-qz edited the summary of this revision.
daiyousei-qz added a comment.

minor naming fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -77,6 +77,7 @@
   C.InlayHints.Parameters = false;
   C.InlayHints.DeducedTypes = false;
   C.InlayHints.Designators = false;
+  C.InlayHints.EndDefinitionComments = false;
   return C;
 }
 
@@ -122,6 +123,17 @@
   assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
 }
 
+template 
+void assertEndDefinitionHints(llvm::StringRef AnnotatedSource,
+  uint32_t MinLines, ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.EndDefinitionComments = true;
+  Cfg.InlayHints.EndDefinitionCommentMinLines = MinLines;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::EndDefinitionComment, AnnotatedSource,
+  Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -1550,6 +1562,165 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(EndDefinitionHints, Functions) {
+  assertEndDefinitionHints(R"cpp(
+$foo[[int foo() {
+  return 41;
+}]]
+$bar[[int bar() {}]]
+
+// No hint because this isn't a definition
+int buz();
+  )cpp",
+   0, ExpectedHint{" /* foo */ ", "foo"},
+   ExpectedHint{" /* bar */ ", "bar"});
+}
+
+TEST(EndDefinitionHints, Methods) {
+  assertEndDefinitionHints(R"cpp(
+class Test {
+public:
+  $ctor[[Test() = default]];
+  $dtor[[~Test() {
+  }]]
+
+  $method[[void method() {}]]
+
+  // No hint because this isn't a definition
+  void method2();
+} x;
+  )cpp",
+   0, ExpectedHint{" /* Test */ ", "ctor"},
+   ExpectedHint{" /* ~Test */ ", "dtor"},
+   ExpectedHint{" /* method */ ", "method"});
+}
+
+TEST(EndDefinitionHints, OverloadedOperators) {
+  assertEndDefinitionHints(R"cpp(
+struct S {
+  $opAdd[[S operator+(int) const {
+return *this;
+  }]]
+
+  $opBool[[operator bool() const {
+return true;
+  }]]
+
+  $opInt[[operator int() const = delete]];
+
+  // No hint because this isn't a definition
+  operator float() const;
+} x;
+  )cpp",
+   0, ExpectedHint{" /* operator+ */ ", "opAdd"},
+   ExpectedHint{" /* operator bool */ ", "opBool"},
+   ExpectedHint{" /* operator int */ ", "opInt"});
+}
+
+TEST(EndDefinitionHints, Namespaces) {
+  assertEndDefinitionHints(
+  R"cpp(
+$anon[[namespace {
+}]]
+
+$ns[[namespace ns {
+  void foo();
+}]]
+  )cpp",
+  0, ExpectedHint{" /* namespace  */ ", "anon"},
+  ExpectedHint{" /* namespace ns */ ", "ns"});
+}
+
+TEST(EndDefinitionHints, Types) {
+  assertEndDefinitionHints(R"cpp(
+$S[[struct S {
+}]];
+
+$C[[class C {
+}]];
+
+$U[[union U {
+}]];
+
+$E1[[enum E1 {
+}]];
+
+$E2[[enum class E2 {
+}]];
+  )cpp",
+   0, ExpectedHint{" /* struct S */ ", "S"},
+   ExpectedHint{" /* class C */ ", "C"},
+   ExpectedHint{" /* union U */ ", "U"},
+   ExpectedHint{" /* enum E1 */ ", "E1"},
+   ExpectedHint{" /* enum class E2 */ ", "E2"});
+}
+
+TEST(EndDefinitionHints, BundledTypeVariableDecl) {
+  assertEndDefinitionHints(
+  R"cpp(
+struct {
+  int x;
+} s;
+
+$anon[[struct {
+  int x;
+}]]
+
+s2;
+  )cpp",
+  0, ExpectedHint{" /* struct  */ ", "anon"});
+}
+
+TEST(EndDefinitionHints, TrailingSemicolon) {
+  assertEndDefinitionHints(R"cpp(
+$S1[[struct S1 {
+}]];
+
+$S2[[struct S2 {
+}]]
+
+;
+
+$S3[[struct S3 {
+}]] ;; ;;
+  )cpp",
+   0, ExpectedHint{" /* struct S1 */ ", "S1"},
+   ExpectedHint{" /* struct S2 */ ", "S2"},
+   ExpectedHint{" /* struct S3 */ ", "S3"});
+}
+

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
daiyousei-qz 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/D150635

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -77,6 +77,7 @@
   C.InlayHints.Parameters = false;
   C.InlayHints.DeducedTypes = false;
   C.InlayHints.Designators = false;
+  C.InlayHints.EndDefinitionComments = false;
   return C;
 }
 
@@ -122,6 +123,17 @@
   assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
 }
 
+template 
+void assertEndDefinitionHints(llvm::StringRef AnnotatedSource,
+  uint32_t MinLines, ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.EndDefinitionComments = true;
+  Cfg.InlayHints.EndDefinitionCommentMinLines = MinLines;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::EndDefinitionComments, AnnotatedSource,
+  Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -1550,6 +1562,165 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(EndDefinitionHints, Functions) {
+  assertEndDefinitionHints(R"cpp(
+$foo[[int foo() {
+  return 41;
+}]]
+$bar[[int bar() {}]]
+
+// No hint because this isn't a definition
+int buz();
+  )cpp",
+   0, ExpectedHint{" /* foo */ ", "foo"},
+   ExpectedHint{" /* bar */ ", "bar"});
+}
+
+TEST(EndDefinitionHints, Methods) {
+  assertEndDefinitionHints(R"cpp(
+class Test {
+public:
+  $ctor[[Test() = default]];
+  $dtor[[~Test() {
+  }]]
+
+  $method[[void method() {}]]
+
+  // No hint because this isn't a definition
+  void method2();
+} x;
+  )cpp",
+   0, ExpectedHint{" /* Test */ ", "ctor"},
+   ExpectedHint{" /* ~Test */ ", "dtor"},
+   ExpectedHint{" /* method */ ", "method"});
+}
+
+TEST(EndDefinitionHints, OverloadedOperators) {
+  assertEndDefinitionHints(R"cpp(
+struct S {
+  $opAdd[[S operator+(int) const {
+return *this;
+  }]]
+
+  $opBool[[operator bool() const {
+return true;
+  }]]
+
+  $opInt[[operator int() const = delete]];
+
+  // No hint because this isn't a definition
+  operator float() const;
+} x;
+  )cpp",
+   0, ExpectedHint{" /* operator+ */ ", "opAdd"},
+   ExpectedHint{" /* operator bool */ ", "opBool"},
+   ExpectedHint{" /* operator int */ ", "opInt"});
+}
+
+TEST(EndDefinitionHints, Namespaces) {
+  assertEndDefinitionHints(
+  R"cpp(
+$anon[[namespace {
+}]]
+
+$ns[[namespace ns {
+  void foo();
+}]]
+  )cpp",
+  0, ExpectedHint{" /* namespace  */ ", "anon"},
+  ExpectedHint{" /* namespace ns */ ", "ns"});
+}
+
+TEST(EndDefinitionHints, Types) {
+  assertEndDefinitionHints(R"cpp(
+$S[[struct S {
+}]];
+
+$C[[class C {
+}]];
+
+$U[[union U {
+}]];
+
+$E1[[enum E1 {
+}]];
+
+$E2[[enum class E2 {
+}]];
+  )cpp",
+   0, ExpectedHint{" /* struct S */ ", "S"},
+   ExpectedHint{" /* class C */ ", "C"},
+   ExpectedHint{" /* union U */ ", "U"},
+   ExpectedHint{" /* enum E1 */ ", "E1"},
+   ExpectedHint{" /* enum class E2 */ ", "E2"});
+}
+
+TEST(EndDefinitionHints, BundledTypeVariableDecl) {
+  assertEndDefinitionHints(
+  R"cpp(
+struct {
+  int x;
+} s;
+
+$anon[[struct {
+  int x;
+}]]
+
+s2;
+  )cpp",
+  0, ExpectedHint{" /* struct  */ ", "anon"});
+}
+
+TEST(EndDefinitionHints, TrailingSemicolon) {
+  assertEndDefinitionHints(R"cpp(
+$S1[[struct S1 {
+}]];
+
+$S2[[struct S2 {
+}]]
+
+;
+
+$S3[[struct S3 {
+}]] ;; ;;
+  )cpp",
+   0, ExpectedHint{" /* struct S1 */ ", "S1"},
+   ExpectedHint{" /* struct S2 */ ", "S2"},
+   ExpectedHint{" /* 

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 521977.
daiyousei-qz added a comment.

Fix format issue


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "Annotations.h"
+#include "Config.h"
 #include "Protocol.h"
 #include "SemanticHighlighting.h"
 #include "SourceCode.h"
@@ -1260,6 +1261,17 @@
   EXPECT_EQ(Toks[3].deltaStart, 2u);
   EXPECT_EQ(Toks[3].length, 3u);
 }
+
+TEST(SemanticHighlighting, WithHighlightingFilter) {
+  llvm::StringRef AnnotatedCode = R"cpp(
+int *$Variable[[x]] = new int;
+)cpp";
+  Config Cfg;
+  Cfg.SemanticTokens.DisabledKinds = {"Operator"};
+  Cfg.SemanticTokens.DisabledModifiers = {"Declaration", "Definition"};
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  checkHighlightings(AnnotatedCode, {}, ~ScopeModifierMask);
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -246,6 +246,23 @@
   EXPECT_EQ(Results[0].InlayHints.DeducedTypes, std::nullopt);
 }
 
+TEST(ParseYAML, SemanticTokens) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+SemanticTokens:
+  DisabledKinds: [ Operator, InactiveCode]
+  DisabledModifiers: Readonly
+  )yaml");
+  auto Results =
+  Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledKinds,
+  ElementsAre(val("Operator"), val("InactiveCode")));
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledModifiers,
+  ElementsAre(val("Readonly")));
+}
+
 TEST(ParseYAML, IncludesIgnoreHeader) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -61,6 +61,8 @@
 };
 
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingKind K);
+std::optional
+highlightingKindFromString(llvm::StringRef Name);
 
 enum class HighlightingModifier {
   Declaration,
@@ -88,6 +90,8 @@
 static_assert(static_cast(HighlightingModifier::LastModifier) < 32,
   "Increase width of modifiers bitfield!");
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingModifier K);
+std::optional
+highlightingModifierFromString(llvm::StringRef Name);
 
 // Contains all information needed for the highlighting a token.
 struct HighlightingToken {
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "SemanticHighlighting.h"
+#include "Config.h"
 #include "FindTarget.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
@@ -354,12 +355,58 @@
   return Winner;
 }
 
+/// Filter to remove particular kinds of highlighting tokens and modifiers from
+/// the output.
+class HighlightingFilter {
+public:
+  HighlightingFilter() {
+for (auto  : ActiveKindLookup)
+  Active = true;
+
+ActiveModifiersMask = ~0;
+  }
+
+  void disableKind(HighlightingKind Kind) {
+ActiveKindLookup[static_cast(Kind)] = false;
+  }
+
+  void disableModifier(HighlightingModifier Modifier) {
+ActiveModifiersMask &= ~(1 << static_cast(Modifier));
+  }
+
+  bool isHighlightKindActive(HighlightingKind Kind) const {
+return ActiveKindLookup[static_cast(Kind)];
+  }
+
+  uint32_t maskModifiers(uint32_t Modifiers) const {
+return Modifiers & ActiveModifiersMask;
+  }
+
+  static HighlightingFilter fromCurrentConfig() {
+const Config  = Config::current();
+HighlightingFilter Filter;
+for (const auto  : C.SemanticTokens.DisabledKinds)
+  if (auto K = 

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 521969.
daiyousei-qz added a comment.

Redo arc as some changes are missing from previous update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "Annotations.h"
+#include "Config.h"
 #include "Protocol.h"
 #include "SemanticHighlighting.h"
 #include "SourceCode.h"
@@ -1260,6 +1261,17 @@
   EXPECT_EQ(Toks[3].deltaStart, 2u);
   EXPECT_EQ(Toks[3].length, 3u);
 }
+
+TEST(SemanticHighlighting, WithHighlightingFilter) {
+  llvm::StringRef AnnotatedCode = R"cpp(
+int *$Variable[[x]] = new int;
+)cpp";
+  Config Cfg;
+  Cfg.SemanticTokens.DisabledKinds = { "Operator" };
+  Cfg.SemanticTokens.DisabledModifiers = { "Declaration", "Definition" };
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  checkHighlightings(AnnotatedCode, {}, ~ScopeModifierMask);
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -246,6 +246,23 @@
   EXPECT_EQ(Results[0].InlayHints.DeducedTypes, std::nullopt);
 }
 
+TEST(ParseYAML, SemanticTokens) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+SemanticTokens:
+  DisabledKinds: [ Operator, InactiveCode]
+  DisabledModifiers: Readonly
+  )yaml");
+  auto Results =
+  Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledKinds,
+  ElementsAre(val("Operator"), val("InactiveCode")));
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledModifiers,
+  ElementsAre(val("Readonly")));
+}
+
 TEST(ParseYAML, IncludesIgnoreHeader) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -61,6 +61,8 @@
 };
 
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingKind K);
+std::optional
+highlightingKindFromString(llvm::StringRef Name);
 
 enum class HighlightingModifier {
   Declaration,
@@ -88,6 +90,8 @@
 static_assert(static_cast(HighlightingModifier::LastModifier) < 32,
   "Increase width of modifiers bitfield!");
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingModifier K);
+std::optional
+highlightingModifierFromString(llvm::StringRef Name);
 
 // Contains all information needed for the highlighting a token.
 struct HighlightingToken {
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "SemanticHighlighting.h"
+#include "Config.h"
 #include "FindTarget.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
@@ -354,12 +355,58 @@
   return Winner;
 }
 
+/// Filter to remove particular kinds of highlighting tokens and modifiers from
+/// the output.
+class HighlightingFilter {
+public:
+  HighlightingFilter() {
+for (auto  : ActiveKindLookup)
+  Active = true;
+
+ActiveModifiersMask = ~0;
+  }
+
+  void disableKind(HighlightingKind Kind) {
+ActiveKindLookup[static_cast(Kind)] = false;
+  }
+
+  void disableModifier(HighlightingModifier Modifier) {
+ActiveModifiersMask &= ~(1 << static_cast(Modifier));
+  }
+
+  bool isHighlightKindActive(HighlightingKind Kind) const {
+return ActiveKindLookup[static_cast(Kind)];
+  }
+
+  uint32_t maskModifiers(uint32_t Modifiers) const {
+return Modifiers & ActiveModifiersMask;
+  }
+
+  static HighlightingFilter fromCurrentConfig() {
+const Config  = Config::current();
+HighlightingFilter Filter;
+for (const auto  : C.SemanticTokens.DisabledKinds)
+   

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 521967.
daiyousei-qz added a comment.



1. Updating D148489 : [clangd] Implement 
configs to stop clangd produce a certain semantic tokens #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Pass filter by value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigFragment.h


Index: clang-tools-extra/clangd/ConfigFragment.h
===
--- clang-tools-extra/clangd/ConfigFragment.h
+++ clang-tools-extra/clangd/ConfigFragment.h
@@ -324,10 +324,11 @@
   };
   InlayHintsBlock InlayHints;
 
-  /// Describes semantic highlighting preferences.
+  /// Configures semantic tokens that are produced by clangd.
   struct SemanticTokensBlock {
+/// Disables clangd to produce semantic tokens for the given kinds.
 std::vector> DisabledKinds;
-
+/// Disables clangd to assign semantic tokens with the given modifiers.
 std::vector> DisabledModifiers;
   };
   SemanticTokensBlock SemanticTokens;
Index: clang-tools-extra/clangd/Config.h
===
--- clang-tools-extra/clangd/Config.h
+++ clang-tools-extra/clangd/Config.h
@@ -150,7 +150,9 @@
   } InlayHints;
 
   struct {
+/// Controls highlighting kinds that are disabled.
 std::vector DisabledKinds;
+/// Controls highlighting modifiers that are disabled.
 std::vector DisabledModifiers;
   } SemanticTokens;
 };


Index: clang-tools-extra/clangd/ConfigFragment.h
===
--- clang-tools-extra/clangd/ConfigFragment.h
+++ clang-tools-extra/clangd/ConfigFragment.h
@@ -324,10 +324,11 @@
   };
   InlayHintsBlock InlayHints;
 
-  /// Describes semantic highlighting preferences.
+  /// Configures semantic tokens that are produced by clangd.
   struct SemanticTokensBlock {
+/// Disables clangd to produce semantic tokens for the given kinds.
 std::vector> DisabledKinds;
-
+/// Disables clangd to assign semantic tokens with the given modifiers.
 std::vector> DisabledModifiers;
   };
   SemanticTokensBlock SemanticTokens;
Index: clang-tools-extra/clangd/Config.h
===
--- clang-tools-extra/clangd/Config.h
+++ clang-tools-extra/clangd/Config.h
@@ -150,7 +150,9 @@
   } InlayHints;
 
   struct {
+/// Controls highlighting kinds that are disabled.
 std::vector DisabledKinds;
+/// Controls highlighting modifiers that are disabled.
 std::vector DisabledModifiers;
   } SemanticTokens;
 };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Sorry, I'm a little occupied lately and don't have time to fix the test 
failure. I'll try to fix that in this week.




Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:400
+  bool ActiveKindLookup[static_cast(HighlightingKind::LastKind) + 1];
+  uint32_t ActiveModifiersMask;
+};

nridge wrote:
> For good measure, let's `static_assert(HighlightingModifier::LastModifier < 
> 32)`
IIRC, we already have such static_assert after the definition of 
HighlightingModifier in SemanticHighlight.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

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

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

In D148489#4276634 , @kadircet wrote:

> before you dive any deeper into the patch, could you give some reasoning 
> about why this is needed/useful?

The discussion is at https://github.com/clangd/clangd/discussions/1598.

The original motivation is that in the latest clangd, we start to assign 
semantic token "operator" to all operators in the source code. Since the 
semantic tokens take priority over the textmate rules in vscode, now we cannot 
theme different operators differently anymore. For example, we cannot color 
"new" in blue while have "+" in white.

To address the issue, this patch adds configurable filter to the semantic token 
to filter out unwanted kinds and modifiers. We should have the following 
benefits with such filter in place.

1. Address the issue mentioned above about unwanted token kinds and modifiers 
so textmate rules could fallback.
2. Improve the performance of semantic tokens (if anyone is taking advantage of 
the config OFC) since we could strip away unwanted tokens
3. Not directly supported by this patch, but the filter could also be used to 
support adding semantic tokens that are disabled by default. Say if we want to 
support `augmentsSyntaxTokens` in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

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

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 514530.
daiyousei-qz added a comment.

  Add some comments to the config headers. Resubmit to retrigger build test 
since I cannot reproduce the failure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "Annotations.h"
+#include "Config.h"
 #include "Protocol.h"
 #include "SemanticHighlighting.h"
 #include "SourceCode.h"
@@ -1260,6 +1261,17 @@
   EXPECT_EQ(Toks[3].deltaStart, 2u);
   EXPECT_EQ(Toks[3].length, 3u);
 }
+
+TEST(SemanticHighlighting, WithHighlightingFilter) {
+  llvm::StringRef AnnotatedCode = R"cpp(
+int *$Variable[[x]] = new int;
+)cpp";
+  Config Cfg;
+  Cfg.SemanticTokens.DisabledKinds = { "Operator" };
+  Cfg.SemanticTokens.DisabledModifiers = { "Declaration", "Definition" };
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  checkHighlightings(AnnotatedCode, {}, ~ScopeModifierMask);
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -246,6 +246,23 @@
   EXPECT_EQ(Results[0].InlayHints.DeducedTypes, std::nullopt);
 }
 
+TEST(ParseYAML, SemanticTokens) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+SemanticTokens:
+  DisabledKinds: [ Operator, InactiveCode]
+  DisabledModifiers: Readonly
+  )yaml");
+  auto Results =
+  Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledKinds,
+  ElementsAre(val("Operator"), val("InactiveCode")));
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledModifiers,
+  ElementsAre(val("Readonly")));
+}
+
 TEST(ParseYAML, IncludesIgnoreHeader) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -61,6 +61,8 @@
 };
 
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingKind K);
+std::optional
+highlightingKindFromString(llvm::StringRef Name);
 
 enum class HighlightingModifier {
   Declaration,
@@ -88,6 +90,8 @@
 static_assert(static_cast(HighlightingModifier::LastModifier) < 32,
   "Increase width of modifiers bitfield!");
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingModifier K);
+std::optional
+highlightingModifierFromString(llvm::StringRef Name);
 
 // Contains all information needed for the highlighting a token.
 struct HighlightingToken {
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "SemanticHighlighting.h"
+#include "Config.h"
 #include "FindTarget.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
@@ -354,12 +355,58 @@
   return Winner;
 }
 
+/// Filter to remove particular kinds of highlighting tokens and modifiers from
+/// the output.
+class HighlightingFilter {
+public:
+  HighlightingFilter() {
+for (auto  : ActiveKindLookup)
+  Active = true;
+
+ActiveModifiersMask = ~0;
+  }
+
+  void disableKind(HighlightingKind Kind) {
+ActiveKindLookup[static_cast(Kind)] = false;
+  }
+
+  void disableModifier(HighlightingModifier Modifier) {
+ActiveModifiersMask &= ~(1 << static_cast(Modifier));
+  }
+
+  bool isHighlightKindActive(HighlightingKind Kind) const {
+return ActiveKindLookup[static_cast(Kind)];
+  }
+
+  uint32_t maskModifiers(uint32_t Modifiers) const {
+return Modifiers & ActiveModifiersMask;
+  }
+
+  static HighlightingFilter fromCurrentConfig() {
+const Config  = Config::current();
+HighlightingFilter Filter;
+   

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:1281
 }
+std::optional
+highlightingKindFromString(llvm::StringRef Name) {

I'm not sure if we should parse the enums here or in parsing configs using 
`EnumSwitch`. The problem is that I don't want to introduce dependencies to 
"SemanticHighlighting.h" in "ConfigCompile.cpp".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

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

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 514088.
daiyousei-qz added a comment.

Fix an issue where the previous change is lost.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "Annotations.h"
+#include "Config.h"
 #include "Protocol.h"
 #include "SemanticHighlighting.h"
 #include "SourceCode.h"
@@ -1260,6 +1261,17 @@
   EXPECT_EQ(Toks[3].deltaStart, 2u);
   EXPECT_EQ(Toks[3].length, 3u);
 }
+
+TEST(SemanticHighlighting, WithHighlightingFilter) {
+  llvm::StringRef AnnotatedCode = R"cpp(
+int *$Variable[[x]] = new int;
+)cpp";
+  Config Cfg;
+  Cfg.SemanticTokens.DisabledKinds = { "Operator" };
+  Cfg.SemanticTokens.DisabledModifiers = { "Declaration", "Definition" };
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  checkHighlightings(AnnotatedCode, {}, ~ScopeModifierMask);
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -246,6 +246,23 @@
   EXPECT_EQ(Results[0].InlayHints.DeducedTypes, std::nullopt);
 }
 
+TEST(ParseYAML, SemanticTokens) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+SemanticTokens:
+  DisabledKinds: [ Operator, InactiveCode]
+  DisabledModifiers: Readonly
+  )yaml");
+  auto Results =
+  Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledKinds,
+  ElementsAre(val("Operator"), val("InactiveCode")));
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledModifiers,
+  ElementsAre(val("Readonly")));
+}
+
 TEST(ParseYAML, IncludesIgnoreHeader) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -61,6 +61,8 @@
 };
 
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingKind K);
+std::optional
+highlightingKindFromString(llvm::StringRef Name);
 
 enum class HighlightingModifier {
   Declaration,
@@ -88,6 +90,8 @@
 static_assert(static_cast(HighlightingModifier::LastModifier) < 32,
   "Increase width of modifiers bitfield!");
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingModifier K);
+std::optional
+highlightingModifierFromString(llvm::StringRef Name);
 
 // Contains all information needed for the highlighting a token.
 struct HighlightingToken {
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "SemanticHighlighting.h"
+#include "Config.h"
 #include "FindTarget.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
@@ -354,12 +355,58 @@
   return Winner;
 }
 
+/// Filter to remove particular kinds of highlighting tokens and modifiers from
+/// the output.
+class HighlightingFilter {
+public:
+  HighlightingFilter() {
+for (auto  : ActiveKindLookup)
+  Active = true;
+
+ActiveModifiersMask = ~0;
+  }
+
+  void disableKind(HighlightingKind Kind) {
+ActiveKindLookup[static_cast(Kind)] = false;
+  }
+
+  void disableModifier(HighlightingModifier Modifier) {
+ActiveModifiersMask &= ~(1 << static_cast(Modifier));
+  }
+
+  bool isHighlightKindActive(HighlightingKind Kind) const {
+return ActiveKindLookup[static_cast(Kind)];
+  }
+
+  uint32_t maskModifiers(uint32_t Modifiers) const {
+return Modifiers & ActiveModifiersMask;
+  }
+
+  static HighlightingFilter fromCurrentConfig() {
+const Config  = Config::current();
+HighlightingFilter Filter;
+for (const auto  : C.SemanticTokens.DisabledKinds)
+  if 

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 514087.
daiyousei-qz added a comment.

Add a test in SemanticHighlightingTests.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

Files:
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -7,6 +7,7 @@
 
//===--===//
 
 #include "Annotations.h"
+#include "Config.h"
 #include "Protocol.h"
 #include "SemanticHighlighting.h"
 #include "SourceCode.h"
@@ -1260,6 +1261,17 @@
   EXPECT_EQ(Toks[3].deltaStart, 2u);
   EXPECT_EQ(Toks[3].length, 3u);
 }
+
+TEST(SemanticHighlighting, WithHighlightingFilter) {
+  llvm::StringRef AnnotatedCode = R"cpp(
+int *$Variable[[x]] = new int;
+)cpp";
+  Config Cfg;
+  Cfg.SemanticTokens.DisabledKinds = { "Operator" };
+  Cfg.SemanticTokens.DisabledModifiers = { "Declaration", "Definition" };
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  checkHighlightings(AnnotatedCode, {}, ~ScopeModifierMask);
+}
 } // namespace
 } // namespace clangd
 } // namespace clang


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "Annotations.h"
+#include "Config.h"
 #include "Protocol.h"
 #include "SemanticHighlighting.h"
 #include "SourceCode.h"
@@ -1260,6 +1261,17 @@
   EXPECT_EQ(Toks[3].deltaStart, 2u);
   EXPECT_EQ(Toks[3].length, 3u);
 }
+
+TEST(SemanticHighlighting, WithHighlightingFilter) {
+  llvm::StringRef AnnotatedCode = R"cpp(
+int *$Variable[[x]] = new int;
+)cpp";
+  Config Cfg;
+  Cfg.SemanticTokens.DisabledKinds = { "Operator" };
+  Cfg.SemanticTokens.DisabledModifiers = { "Declaration", "Definition" };
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  checkHighlightings(AnnotatedCode, {}, ~ScopeModifierMask);
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 514086.
daiyousei-qz added a comment.

Fix unittest


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -246,6 +246,23 @@
   EXPECT_EQ(Results[0].InlayHints.DeducedTypes, std::nullopt);
 }
 
+TEST(ParseYAML, SemanticTokens) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+SemanticTokens:
+  DisabledKinds: [ Operator, InactiveCode]
+  DisabledModifiers: Readonly
+  )yaml");
+  auto Results =
+  Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledKinds,
+  ElementsAre(val("Operator"), val("InactiveCode")));
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledModifiers,
+  ElementsAre(val("Readonly")));
+}
+
 TEST(ParseYAML, IncludesIgnoreHeader) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -61,6 +61,8 @@
 };
 
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingKind K);
+std::optional
+highlightingKindFromString(llvm::StringRef Name);
 
 enum class HighlightingModifier {
   Declaration,
@@ -88,6 +90,8 @@
 static_assert(static_cast(HighlightingModifier::LastModifier) < 32,
   "Increase width of modifiers bitfield!");
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingModifier K);
+std::optional
+highlightingModifierFromString(llvm::StringRef Name);
 
 // Contains all information needed for the highlighting a token.
 struct HighlightingToken {
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "SemanticHighlighting.h"
+#include "Config.h"
 #include "FindTarget.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
@@ -354,12 +355,58 @@
   return Winner;
 }
 
+/// Filter to remove particular kinds of highlighting tokens and modifiers from
+/// the output.
+class HighlightingFilter {
+public:
+  HighlightingFilter() {
+for (auto  : ActiveKindLookup)
+  Active = true;
+
+ActiveModifiersMask = ~0;
+  }
+
+  void disableKind(HighlightingKind Kind) {
+ActiveKindLookup[static_cast(Kind)] = false;
+  }
+
+  void disableModifier(HighlightingModifier Modifier) {
+ActiveModifiersMask &= ~(1 << static_cast(Modifier));
+  }
+
+  bool isHighlightKindActive(HighlightingKind Kind) const {
+return ActiveKindLookup[static_cast(Kind)];
+  }
+
+  uint32_t maskModifiers(uint32_t Modifiers) const {
+return Modifiers & ActiveModifiersMask;
+  }
+
+  static HighlightingFilter fromCurrentConfig() {
+const Config  = Config::current();
+HighlightingFilter Filter;
+for (const auto  : C.SemanticTokens.DisabledKinds)
+  if (auto K = highlightingKindFromString(Kind))
+Filter.disableKind(*K);
+for (const auto  : C.SemanticTokens.DisabledModifiers)
+  if (auto M = highlightingModifierFromString(Modifier))
+Filter.disableModifier(*M);
+
+return Filter;
+  }
+
+private:
+  bool ActiveKindLookup[static_cast(HighlightingKind::LastKind) + 1];
+  uint32_t ActiveModifiersMask;
+};
+
 /// Consumes source locations and maps them to text ranges for highlightings.
 class HighlightingsBuilder {
 public:
-  HighlightingsBuilder(const ParsedAST , bool IncludeInactiveRegionTokens)
+  HighlightingsBuilder(const ParsedAST , const HighlightingFilter ,
+   bool IncludeInactiveRegionTokens)
   : TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),
-LangOpts(AST.getLangOpts()),
+LangOpts(AST.getLangOpts()), Filter(Filter),
 IncludeInactiveRegionTokens(IncludeInactiveRegionTokens) {}
 
   HighlightingToken (SourceLocation Loc, HighlightingKind Kind) {
@@ -412,6 +459,9 @@
   }
 
   HighlightingToken (Range R, HighlightingKind Kind) {
+if (!Filter.isHighlightKindActive(Kind))
+  return InvalidHighlightingToken;
+
 

[PATCH] D148489: [clangd] Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Please hold off from reviewing this. I have been figuring out how arc tools 
work and the unittest of the current version is broken.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148489

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

[PATCH] D148489: Implement configs to stop clangd produce a certain semantic tokens

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
daiyousei-qz 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/D148489

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -246,6 +246,24 @@
   EXPECT_EQ(Results[0].InlayHints.DeducedTypes, std::nullopt);
 }
 
+TEST(ParseYAML, SemanticTokens) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+SemanticTokens:
+  DisabledKinds: [ Operator, InactiveCode]
+  DisabledModifiers: Readonly
+  )yaml");
+  auto Results =
+  Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  EXPECT_THAT(
+  Results[0].SemanticTokens.DisabledKinds,
+  llvm::ValueIs(val(std::vector{"Operator", "InactiveCode"})));
+  EXPECT_THAT(Results[0].SemanticTokens.DisabledModifiers,
+  llvm::ValueIs(val(std::vector{"Readonly"})));
+}
+
 TEST(ParseYAML, IncludesIgnoreHeader) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -61,6 +61,8 @@
 };
 
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingKind K);
+std::optional
+highlightingKindFromString(llvm::StringRef Name);
 
 enum class HighlightingModifier {
   Declaration,
@@ -88,6 +90,8 @@
 static_assert(static_cast(HighlightingModifier::LastModifier) < 32,
   "Increase width of modifiers bitfield!");
 llvm::raw_ostream <<(llvm::raw_ostream , HighlightingModifier K);
+std::optional
+highlightingModifierFromString(llvm::StringRef Name);
 
 // Contains all information needed for the highlighting a token.
 struct HighlightingToken {
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "SemanticHighlighting.h"
+#include "Config.h"
 #include "FindTarget.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
@@ -354,12 +355,58 @@
   return Winner;
 }
 
+/// Filter to remove particular kinds of highlighting tokens and modifiers from
+/// the output.
+class HighlightingFilter {
+public:
+  HighlightingFilter() {
+for (auto  : ActiveKindLookup)
+  Active = true;
+
+ActiveModifiersMask = ~0;
+  }
+
+  void disableKind(HighlightingKind Kind) {
+ActiveKindLookup[static_cast(Kind)] = false;
+  }
+
+  void disableModifier(HighlightingModifier Modifier) {
+ActiveModifiersMask &= ~(1 << static_cast(Modifier));
+  }
+
+  bool isHighlightKindActive(HighlightingKind Kind) const {
+return ActiveKindLookup[static_cast(Kind)];
+  }
+
+  uint32_t maskModifiers(uint32_t Modifiers) const {
+return Modifiers & ActiveModifiersMask;
+  }
+
+  static HighlightingFilter fromCurrentConfig() {
+const Config  = Config::current();
+HighlightingFilter Filter;
+for (const auto  : C.SemanticTokens.DisabledKinds)
+  if (auto K = highlightingKindFromString(Kind))
+Filter.disableKind(*K);
+for (const auto  : C.SemanticTokens.DisabledModifiers)
+  if (auto M = highlightingModifierFromString(Modifier))
+Filter.disableModifier(*M);
+
+return Filter;
+  }
+
+private:
+  bool ActiveKindLookup[static_cast(HighlightingKind::LastKind) + 1];
+  uint32_t ActiveModifiersMask;
+};
+
 /// Consumes source locations and maps them to text ranges for highlightings.
 class HighlightingsBuilder {
 public:
-  HighlightingsBuilder(const ParsedAST , bool IncludeInactiveRegionTokens)
+  HighlightingsBuilder(const ParsedAST , const HighlightingFilter ,
+   bool IncludeInactiveRegionTokens)
   : TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),
-LangOpts(AST.getLangOpts()),
+LangOpts(AST.getLangOpts()), Filter(Filter),
 IncludeInactiveRegionTokens(IncludeInactiveRegionTokens) {}
 
   HighlightingToken (SourceLocation Loc, HighlightingKind Kind) {
@@ -412,6 +459,9 @@
   }
 
   HighlightingToken 

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

In D127082#4089377 , @nridge wrote:

> @daiyousei-qz what is the current status of this patch? Is it ready to be 
> merged again? (If so, I can do that for you.)

Honestly, I don't know. I looked at TOT llvm repo in github and it looks like 
this patch is already there. I guess no further action is needed?


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

[PATCH] D136033: Add an option to specify a workspece-level config file

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz created this revision.
daiyousei-qz added reviewers: nridge, kadircet.
Herald added a subscriber: arphaman.
Herald added a project: All.
daiyousei-qz requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added a project: clang-tools-extra.

Add an option "--workspace-config=" to specify a workspace config file, 
which stands between the in-tree .clangd and the global user config file and 
offers per-workspace customization.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136033

Files:
  clang-tools-extra/clangd/tool/ClangdMain.cpp


Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -491,8 +491,9 @@
 "enable-config",
 cat(Misc),
 desc(
-"Read user and project configuration from YAML files.\n"
+"Read user/workspace/project configuration from YAML files.\n"
 "Project config is from a .clangd file in the project directory.\n"
+"Workspace config is specified by the --workspace-config option.\n"
 "User config is from clangd/config.yaml in the following 
directories:\n"
 "\tWindows: %USERPROFILE%\\AppData\\Local\n"
 "\tMac OS: ~/Library/Preferences/\n"
@@ -501,6 +502,13 @@
 init(true),
 };
 
+opt WorkspaceConfig{
+"workspace-config",
+cat(Misc),
+desc("Path to a workspace configuration file."),
+init(""),
+};
+
 opt UseDirtyHeaders{"use-dirty-headers", cat(Misc),
   desc("Use files open in the editor when parsing "
"headers instead of reading from the disk"),
@@ -923,6 +931,15 @@
   if (EnableConfig) {
 ProviderStack.push_back(
 config::Provider::fromAncestorRelativeYAMLFiles(".clangd", TFS));
+if (!WorkspaceConfig.empty() && llvm::sys::fs::exists(WorkspaceConfig)) {
+  llvm::SmallString<256> AbsWorkspaceConfigPath =
+  StringRef{WorkspaceConfig.getValue()};
+  llvm::sys::fs::make_absolute("", AbsWorkspaceConfigPath);
+  ProviderStack.push_back(config::Provider::fromYAMLFile(
+  AbsWorkspaceConfigPath, /*Directory=*/"", TFS, /*Trusted=*/true));
+} else {
+  elog("Couldn't find workspace config file, not loading");
+}
 llvm::SmallString<256> UserConfig;
 if (llvm::sys::path::user_config_directory(UserConfig)) {
   llvm::sys::path::append(UserConfig, "clangd", "config.yaml");


Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -491,8 +491,9 @@
 "enable-config",
 cat(Misc),
 desc(
-"Read user and project configuration from YAML files.\n"
+"Read user/workspace/project configuration from YAML files.\n"
 "Project config is from a .clangd file in the project directory.\n"
+"Workspace config is specified by the --workspace-config option.\n"
 "User config is from clangd/config.yaml in the following directories:\n"
 "\tWindows: %USERPROFILE%\\AppData\\Local\n"
 "\tMac OS: ~/Library/Preferences/\n"
@@ -501,6 +502,13 @@
 init(true),
 };
 
+opt WorkspaceConfig{
+"workspace-config",
+cat(Misc),
+desc("Path to a workspace configuration file."),
+init(""),
+};
+
 opt UseDirtyHeaders{"use-dirty-headers", cat(Misc),
   desc("Use files open in the editor when parsing "
"headers instead of reading from the disk"),
@@ -923,6 +931,15 @@
   if (EnableConfig) {
 ProviderStack.push_back(
 config::Provider::fromAncestorRelativeYAMLFiles(".clangd", TFS));
+if (!WorkspaceConfig.empty() && llvm::sys::fs::exists(WorkspaceConfig)) {
+  llvm::SmallString<256> AbsWorkspaceConfigPath =
+  StringRef{WorkspaceConfig.getValue()};
+  llvm::sys::fs::make_absolute("", AbsWorkspaceConfigPath);
+  ProviderStack.push_back(config::Provider::fromYAMLFile(
+  AbsWorkspaceConfigPath, /*Directory=*/"", TFS, /*Trusted=*/true));
+} else {
+  elog("Couldn't find workspace config file, not loading");
+}
 llvm::SmallString<256> UserConfig;
 if (llvm::sys::path::user_config_directory(UserConfig)) {
   llvm::sys::path::append(UserConfig, "clangd", "config.yaml");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

In D127082#3776173 , @vitalybuka 
wrote:

> Looks like it breaks
> https://lab.llvm.org/buildbot/#/builders/236/builds/206
> https://lab.llvm.org/buildbot/#/builders/237/builds/85
> https://lab.llvm.org/buildbot/#/builders/238/builds/103

I take a quick look and it seems the failing tests were timed out. Considering 
the context of the change, I would guess it might be caused by some huge, 
nested macro being expanded. But it's weird since the expansion should only 
happen for Hover. Unfortunately, I don't have any aarch64 device to reproduce 
this locally and investigate further. Please feel free to reach out to me if 
there's anything I could help.


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

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

In D127082#3770763 , @sammccall wrote:

> Sorry for the delay :-( This looks great!
>
> It looks like this is your first patch, so you don't have commit access. I 
> can land the patch for you.
> Can you provide an email address for attribution?

I had a patch for clang which Nathan had merged for me. Probably it's because 
of the lack of the metadata since I was using the web interface to submit 
differentials. Anyway, here's the info:
Qingyuan Zheng qyzhe...@outlook.com


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

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

@sammccall Hello, is there any update required by me?


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

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 447962.
daiyousei-qz added a comment.

remove dedicated MacroExpansion variable (reuse Definition)
added empty MACRO test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082

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
@@ -478,15 +478,60 @@
  HI.Definition = "Foo";
}},
 
-  // macro
+  // empty macro
+  {R"cpp(
+#define MACRO
+[[MAC^RO]]
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO";
+   }},
+
+  // object-like macro
+  {R"cpp(
+#define MACRO 41
+int x = [[MAC^RO]];
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO 41\n\n"
+ "// Expands to\n"
+ "41";
+   }},
+
+  // function-like macro
   {R"cpp(
 // Best MACRO ever.
-#define MACRO(x,y,z) void foo(x, y, z);
+#define MACRO(x,y,z) void foo(x, y, z)
 [[MAC^RO]](int, double d, bool z = false);
 )cpp",
[](HoverInfo ) {
- HI.Name = "MACRO", HI.Kind = index::SymbolKind::Macro,
- HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z);";
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z)\n\n"
+ "// Expands to\n"
+ "void foo(int, double d, bool z = false)";
+   }},
+
+  // nested macro
+  {R"cpp(
+#define STRINGIFY_AUX(s) #s
+#define STRINGIFY(s) STRINGIFY_AUX(s)
+#define DECL_STR(NAME, VALUE) const char *v_##NAME = STRINGIFY(VALUE)
+#define FOO 41
+
+[[DECL^_STR]](foo, FOO);
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "DECL_STR";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
+ "STRINGIFY(VALUE)\n\n"
+ "// Expands to\n"
+ "const char *v_foo = \"41\"";
}},
 
   // constexprs
@@ -1566,7 +1611,9 @@
   [](HoverInfo ) {
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
-HI.Definition = "#define MACRO 0";
+HI.Definition = "#define MACRO 0\n\n"
+"// Expands to\n"
+"0";
   }},
   {
   R"cpp(// Macro
@@ -1577,6 +1624,8 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+// NOTE MACRO doesn't have expansion since it technically isn't
+// expanded here
   }},
   {
   R"cpp(// Macro
@@ -1590,7 +1639,10 @@
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition =
 R"cpp(#define MACRO  \
-  { return 0; })cpp";
+  { return 0; }
+
+// Expands to
+{ return 0; })cpp";
   }},
   {
   R"cpp(// Forward class declaration
@@ -2986,6 +3038,21 @@
 
 // In test::Bar
 int foo = 3)",
+  },
+  {
+  [](HoverInfo ) {
+HI.Kind = index::SymbolKind::Macro;
+HI.Name = "PLUS_ONE";
+HI.Definition = "#define PLUS_ONE(X) (X+1)\n\n"
+"// Expands to\n"
+"(1 + 1)";
+  },
+  R"(macro PLUS_ONE
+
+#define PLUS_ONE(X) (X+1)
+
+// Expands to
+(1 + 1))",
   },
   {
   [](HoverInfo ) {
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -641,7 +641,8 @@
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
-HoverInfo getHoverContents(const DefinedMacro , ParsedAST ) {
+HoverInfo getHoverContents(const syntax::Token , const DefinedMacro ,
+   ParsedAST ) {
   HoverInfo HI;
   SourceManager  = AST.getSourceManager();
   HI.Name = std::string(Macro.Name);
@@ -672,6 +673,29 @@
 .str();
 }
   }
+
+  if (auto Expansion = AST.getTokens().expansionStartingAt()) {
+// We drop expansion that's longer than the threshold.
+// For extremely long expansion text, it's not readable from hover card
+// anyway.
+

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:1187
 
-  if (!Definition.empty()) {
+  if (!Definition.empty() || !MacroExpansion.empty()) {
 Output.addRuler();

sammccall wrote:
> Sorry, I think I wasn't clear about what I was asking for...
> Rather than merging `HoverInfo::MacroExpansion` and `HoverInfo::Definition` 
> during `present()`, I'd really like to eliminate `HoverInfo::MacroExpansion` 
> entirely, and have `HoverInfo::Definition` be:
> ```
> #define FOO BAR
> 
> // Expands to
> BAR
> ```
> 
> This isn't completely consistent with the way scope/access are handled, but 
> those are somewhat more generic features and I'd rather avoid adding new 
> HoverInfo members specific to macros.
> 
> (The final presentation does look good, so this is just about the 
> intermediate HoverInfo struct)
I see. Though I'm not sure if putting both of them in a single string will 
cause problems when doing the format. That is:
```
#define MACRO token token token token token

// Expands to
token token token token token token token token token
```
as a whole will be reformatted by clang-format and I'm not sure if the overall 
layout could always be preserved. Let me do some expriment tomorrow.


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

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

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 

F23848497: image.png 

F23848508: image.png 




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

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 446041.
daiyousei-qz added a comment.

resolve review comment

produce expansion even if definition isn't available


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  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
@@ -478,15 +478,46 @@
  HI.Definition = "Foo";
}},
 
-  // macro
+  // object-like macro
+  {R"cpp(
+#define MACRO 41
+int x = [[MAC^RO]];
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+   }},
+
+  // function-like macro
   {R"cpp(
 // Best MACRO ever.
-#define MACRO(x,y,z) void foo(x, y, z);
+#define MACRO(x,y,z) void foo(x, y, z)
 [[MAC^RO]](int, double d, bool z = false);
 )cpp",
[](HoverInfo ) {
- HI.Name = "MACRO", HI.Kind = index::SymbolKind::Macro,
- HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z);";
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z)";
+ HI.MacroExpansion = "void foo(int, double d, bool z = false)";
+   }},
+
+  // nested macro
+  {R"cpp(
+#define STRINGIFY_AUX(s) #s
+#define STRINGIFY(s) STRINGIFY_AUX(s)
+#define DECL_STR(NAME, VALUE) const char *v_##NAME = STRINGIFY(VALUE)
+#define FOO 41
+
+[[DECL^_STR]](foo, FOO);
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "DECL_STR";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
+ "STRINGIFY(VALUE)";
+ HI.MacroExpansion = "const char *v_foo = \"41\"";
}},
 
   // constexprs
@@ -1070,6 +1101,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
@@ -1567,6 +1599,7 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+HI.MacroExpansion = "0";
   }},
   {
   R"cpp(// Macro
@@ -1577,6 +1610,8 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+// NOTE MACRO doesn't have expansion since it technically isn't
+// expanded here
   }},
   {
   R"cpp(// Macro
@@ -1591,6 +1626,7 @@
 HI.Definition =
 R"cpp(#define MACRO  \
   { return 0; })cpp";
+HI.MacroExpansion = "{ return 0; }";
   }},
   {
   R"cpp(// Forward class declaration
@@ -2625,6 +2661,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
@@ -2986,6 +3023,20 @@
 
 // In test::Bar
 int foo = 3)",
+  },
+  {
+  [](HoverInfo ) {
+HI.Kind = index::SymbolKind::Macro;
+HI.Name = "PLUS_ONE";
+HI.Definition = "#define PLUS_ONE(X) (X+1)";
+HI.MacroExpansion = "(1 + 1)";
+  },
+  R"(macro PLUS_ONE
+
+#define PLUS_ONE(X) (X+1)
+
+// Expands to
+(1 + 1))",
   },
   {
   [](HoverInfo ) {
Index: clang-tools-extra/clangd/Hover.h
===
--- clang-tools-extra/clangd/Hover.h
+++ clang-tools-extra/clangd/Hover.h
@@ -71,6 +71,7 @@
   std::string Documentation;
   /// Source code containing the definition of the symbol.
   std::string Definition;
+  std::string MacroExpansion;
   const char *DefinitionLanguage = "cpp";
   /// Access specifier for declarations inside class/struct/unions, empty for
   /// others.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ 

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 444151.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  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
@@ -478,15 +478,46 @@
  HI.Definition = "Foo";
}},
 
-  // macro
+  // variable-like macro
+  {R"cpp(
+#define MACRO 41
+int x = [[MAC^RO]];
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+   }},
+
+  // function-like macro
   {R"cpp(
 // Best MACRO ever.
-#define MACRO(x,y,z) void foo(x, y, z);
+#define MACRO(x,y,z) void foo(x, y, z)
 [[MAC^RO]](int, double d, bool z = false);
 )cpp",
[](HoverInfo ) {
- HI.Name = "MACRO", HI.Kind = index::SymbolKind::Macro,
- HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z);";
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z)";
+ HI.MacroExpansion = "void foo(int, double d, bool z = false)";
+   }},
+
+  // nested macro
+  {R"cpp(
+#define STRINGIFY_AUX(s) #s
+#define STRINGIFY(s) STRINGIFY_AUX(s)
+#define DECL_STR(NAME, VALUE) const char *v_##NAME = STRINGIFY(VALUE)
+#define FOO 41
+
+[[DECL^_STR]](foo, FOO);
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "DECL_STR";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
+ "STRINGIFY(VALUE)";
+ HI.MacroExpansion = "const char *v_foo = \"41\"";
}},
 
   // constexprs
@@ -1070,6 +1101,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
@@ -1567,6 +1599,7 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+HI.MacroExpansion = "0";
   }},
   {
   R"cpp(// Macro
@@ -1577,6 +1610,8 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+// FIXME: expansion of MACRO isn't available in macro
+// definition/arguments
   }},
   {
   R"cpp(// Macro
@@ -1591,6 +1626,7 @@
 HI.Definition =
 R"cpp(#define MACRO  \
   { return 0; })cpp";
+HI.MacroExpansion = "{ return 0; }";
   }},
   {
   R"cpp(// Forward class declaration
@@ -2625,6 +2661,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
Index: clang-tools-extra/clangd/Hover.h
===
--- clang-tools-extra/clangd/Hover.h
+++ clang-tools-extra/clangd/Hover.h
@@ -71,6 +71,7 @@
   std::string Documentation;
   /// Source code containing the definition of the symbol.
   std::string Definition;
+  std::string MacroExpansion;
   const char *DefinitionLanguage = "cpp";
   /// Access specifier for declarations inside class/struct/unions, empty for
   /// others.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -641,7 +641,8 @@
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
-HoverInfo getHoverContents(const DefinedMacro , ParsedAST ) {
+HoverInfo getHoverContents(const syntax::Token , const DefinedMacro ,
+   ParsedAST ) {
   HoverInfo HI;
   SourceManager  = AST.getSourceManager();
   HI.Name = std::string(Macro.Name);
@@ -670,6 +671,28 @@
 HI.Definition =
 ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))
 .str();
+
+  if (auto Expansion = 

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 444150.
daiyousei-qz added a comment.
Herald added subscribers: Sanitizers, Enna1.
Herald added a project: Sanitizers.

fix patch context


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp

Index: compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
===
--- compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
+++ compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
@@ -223,10 +223,10 @@
 if (GetMacosAlignedVersion() >= MacosVersion(13, 0)) {
   dyld_hdr = GetDyldImageHeaderViaSharedCache();
   if (!dyld_hdr) {
-Printf(
-"Failed to lookup the dyld image header in the shared cache on "
-"macOS 13+ (or no shared cache in use).  Falling back to lookup via"
-"vm_region_recurse_64().\n");
+VReport(1,
+"Failed to lookup the dyld image header in the shared cache on "
+"macOS 13+ (or no shared cache in use).  Falling back to "
+"lookup via vm_region_recurse_64().\n");
 dyld_hdr = GetDyldImageHeaderViaVMRegion();
   }
 } else {
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -478,15 +478,46 @@
  HI.Definition = "Foo";
}},
 
-  // macro
+  // variable-like macro
+  {R"cpp(
+#define MACRO 41
+int x = [[MAC^RO]];
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+   }},
+
+  // function-like macro
   {R"cpp(
 // Best MACRO ever.
-#define MACRO(x,y,z) void foo(x, y, z);
+#define MACRO(x,y,z) void foo(x, y, z)
 [[MAC^RO]](int, double d, bool z = false);
 )cpp",
[](HoverInfo ) {
- HI.Name = "MACRO", HI.Kind = index::SymbolKind::Macro,
- HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z);";
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z)";
+ HI.MacroExpansion = "void foo(int, double d, bool z = false)";
+   }},
+
+  // nested macro
+  {R"cpp(
+#define STRINGIFY_AUX(s) #s
+#define STRINGIFY(s) STRINGIFY_AUX(s)
+#define DECL_STR(NAME, VALUE) const char *v_##NAME = STRINGIFY(VALUE)
+#define FOO 41
+
+[[DECL^_STR]](foo, FOO);
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "DECL_STR";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
+ "STRINGIFY(VALUE)";
+ HI.MacroExpansion = "const char *v_foo = \"41\"";
}},
 
   // constexprs
@@ -1070,6 +1101,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
@@ -1567,6 +1599,7 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+HI.MacroExpansion = "0";
   }},
   {
   R"cpp(// Macro
@@ -1577,6 +1610,8 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+// FIXME: expansion of MACRO isn't available in macro
+// definition/arguments
   }},
   {
   R"cpp(// Macro
@@ -1591,6 +1626,7 @@
 HI.Definition =
 R"cpp(#define MACRO  \
   { return 0; })cpp";
+HI.MacroExpansion = "{ return 0; }";
   }},
   {
   R"cpp(// Forward class declaration
@@ -2625,6 +2661,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
Index: clang-tools-extra/clangd/Hover.h

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 444147.
daiyousei-qz added a comment.

fix line end to LF


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  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
@@ -478,15 +478,46 @@
  HI.Definition = "Foo";
}},
 
-  // macro
+  // variable-like macro
+  {R"cpp(
+#define MACRO 41
+int x = [[MAC^RO]];
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+   }},
+
+  // function-like macro
   {R"cpp(
 // Best MACRO ever.
-#define MACRO(x,y,z) void foo(x, y, z);
+#define MACRO(x,y,z) void foo(x, y, z)
 [[MAC^RO]](int, double d, bool z = false);
 )cpp",
[](HoverInfo ) {
- HI.Name = "MACRO", HI.Kind = index::SymbolKind::Macro,
- HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z);";
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z)";
+ HI.MacroExpansion = "void foo(int, double d, bool z = false)";
+   }},
+
+  // nested macro
+  {R"cpp(
+#define STRINGIFY_AUX(s) #s
+#define STRINGIFY(s) STRINGIFY_AUX(s)
+#define DECL_STR(NAME, VALUE) const char *v_##NAME = STRINGIFY(VALUE)
+#define FOO 41
+
+[[DECL^_STR]](foo, FOO);
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "DECL_STR";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
+ "STRINGIFY(VALUE)";
+ HI.MacroExpansion = "const char *v_foo = \"41\"";
}},
 
   // constexprs
@@ -1070,6 +1101,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
@@ -1567,6 +1599,7 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+HI.MacroExpansion = "0";
   }},
   {
   R"cpp(// Macro
@@ -1577,6 +1610,8 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+// FIXME: expansion of MACRO isn't available in macro
+// definition/arguments
   }},
   {
   R"cpp(// Macro
@@ -1591,6 +1626,7 @@
 HI.Definition =
 R"cpp(#define MACRO  \
   { return 0; })cpp";
+HI.MacroExpansion = "{ return 0; }";
   }},
   {
   R"cpp(// Forward class declaration
@@ -2625,6 +2661,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
Index: clang-tools-extra/clangd/Hover.h
===
--- clang-tools-extra/clangd/Hover.h
+++ clang-tools-extra/clangd/Hover.h
@@ -71,6 +71,7 @@
   std::string Documentation;
   /// Source code containing the definition of the symbol.
   std::string Definition;
+  std::string MacroExpansion;
   const char *DefinitionLanguage = "cpp";
   /// Access specifier for declarations inside class/struct/unions, empty for
   /// others.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -641,7 +641,8 @@
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
-HoverInfo getHoverContents(const DefinedMacro , ParsedAST ) {
+HoverInfo getHoverContents(const syntax::Token , const DefinedMacro ,
+   ParsedAST ) {
   HoverInfo HI;
   SourceManager  = AST.getSourceManager();
   HI.Name = std::string(Macro.Name);
@@ -670,6 +671,28 @@
 HI.Definition =
 ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

It looks like my inline comment wasn't submitted (didn't click the submit 
button in the bottom). Here's my old comment.




Comment at: clang-tools-extra/clangd/Hover.cpp:1091
+
+  // Reformat Macro Expansion
+  if (!HI->MacroExpansion.empty()) {

nridge wrote:
> It would be interesting to have a couple of test cases that exercise the 
> reformatting in non-trivial ways, e.g. long expansions that need to be 
> wrapped onto multiple lines
> 
> I would suggest two such test cases, one with the expansion being in a 
> declaration context, and the second an expression context (for this one, to 
> make it long enough, the expansion could contain e.g. an `a ? b : c` 
> expression)
> 
> (I'm suggesting the expression-context testcase in part as a sanity check to 
> make sure that `format::reformat()` handles such code reasonably in the first 
> place)
Somehow, this comment goes out of the position. 

In my opinion, such test should be written against `format::reformat()` 
directly instead of hover message in clangd. One problem is that we are using 
the current style in users' workspace to reformat the definition/expansion, 
which means the same tokens might present differently given different 
`.clang-format` or fallback style that the user has specified. I do agree that, 
if tokens don't conform a regular c++ expression, like `) . field`, the 
presentation could be bad. But I suppose there's no obvious solution for that.


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

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 442416.
daiyousei-qz marked an inline comment as done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  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
@@ -478,15 +478,46 @@
  HI.Definition = "Foo";
}},
 
-  // macro
+  // variable-like macro
+  {R"cpp(
+#define MACRO 41
+int x = [[MAC^RO]];
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+   }},
+
+  // function-like macro
   {R"cpp(
 // Best MACRO ever.
-#define MACRO(x,y,z) void foo(x, y, z);
+#define MACRO(x,y,z) void foo(x, y, z)
 [[MAC^RO]](int, double d, bool z = false);
 )cpp",
[](HoverInfo ) {
- HI.Name = "MACRO", HI.Kind = index::SymbolKind::Macro,
- HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z);";
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z)";
+ HI.MacroExpansion = "void foo(int, double d, bool z = false)";
+   }},
+
+  // nested macro
+  {R"cpp(
+#define STRINGIFY_AUX(s) #s
+#define STRINGIFY(s) STRINGIFY_AUX(s)
+#define DECL_STR(NAME, VALUE) const char *v_##NAME = STRINGIFY(VALUE)
+#define FOO 41
+
+[[DECL^_STR]](foo, FOO);
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "DECL_STR";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
+ "STRINGIFY(VALUE)";
+ HI.MacroExpansion = "const char *v_foo = \"41\"";
}},
 
   // constexprs
@@ -1070,6 +1101,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
@@ -1567,6 +1599,7 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+HI.MacroExpansion = "0";
   }},
   {
   R"cpp(// Macro
@@ -1577,6 +1610,8 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+// FIXME: expansion of MACRO isn't available in macro
+// definition/arguments
   }},
   {
   R"cpp(// Macro
@@ -1591,6 +1626,7 @@
 HI.Definition =
 R"cpp(#define MACRO  \
   { return 0; })cpp";
+HI.MacroExpansion = "{ return 0; }";
   }},
   {
   R"cpp(// Forward class declaration
@@ -2625,6 +2661,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
Index: clang-tools-extra/clangd/Hover.h
===
--- clang-tools-extra/clangd/Hover.h
+++ clang-tools-extra/clangd/Hover.h
@@ -71,6 +71,7 @@
   std::string Documentation;
   /// Source code containing the definition of the symbol.
   std::string Definition;
+  std::string MacroExpansion;
   const char *DefinitionLanguage = "cpp";
   /// Access specifier for declarations inside class/struct/unions, empty for
   /// others.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -641,7 +641,8 @@
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
-HoverInfo getHoverContents(const DefinedMacro , ParsedAST ) {
+HoverInfo getHoverContents(const syntax::Token , const DefinedMacro ,
+   ParsedAST ) {
   HoverInfo HI;
   SourceManager  = AST.getSourceManager();
   HI.Name = std::string(Macro.Name);
@@ -670,6 +671,28 @@
 HI.Definition =
 ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 441920.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  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
@@ -478,15 +478,46 @@
  HI.Definition = "Foo";
}},
 
-  // macro
+  // variable-like macro
+  {R"cpp(
+#define MACRO 41
+int x = [[MAC^RO]];
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+   }},
+
+  // function-like macro
   {R"cpp(
 // Best MACRO ever.
-#define MACRO(x,y,z) void foo(x, y, z);
+#define MACRO(x,y,z) void foo(x, y, z)
 [[MAC^RO]](int, double d, bool z = false);
 )cpp",
[](HoverInfo ) {
- HI.Name = "MACRO", HI.Kind = index::SymbolKind::Macro,
- HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z);";
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z)";
+ HI.MacroExpansion = "void foo(int, double d, bool z = false)";
+   }},
+
+  // nested macro
+  {R"cpp(
+#define STRINGIFY_AUX(s) #s
+#define STRINGIFY(s) STRINGIFY_AUX(s)
+#define DECL_STR(NAME, VALUE) const char *v_##NAME = STRINGIFY(VALUE)
+#define FOO 41
+
+[[DECL^_STR]](foo, FOO);
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "DECL_STR";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
+ "STRINGIFY(VALUE)";
+ HI.MacroExpansion = "const char *v_foo = \"41\"";
}},
 
   // constexprs
@@ -1070,6 +1101,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
@@ -1567,6 +1599,7 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+HI.MacroExpansion = "0";
   }},
   {
   R"cpp(// Macro
@@ -1577,6 +1610,8 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+// FIXME: expansion of MACRO isn't available in macro
+// definition/arguments
   }},
   {
   R"cpp(// Macro
@@ -1591,6 +1626,7 @@
 HI.Definition =
 R"cpp(#define MACRO  \
   { return 0; })cpp";
+HI.MacroExpansion = "{ return 0; }";
   }},
   {
   R"cpp(// Forward class declaration
@@ -2625,6 +2661,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
Index: clang-tools-extra/clangd/Hover.h
===
--- clang-tools-extra/clangd/Hover.h
+++ clang-tools-extra/clangd/Hover.h
@@ -71,6 +71,7 @@
   std::string Documentation;
   /// Source code containing the definition of the symbol.
   std::string Definition;
+  std::string MacroExpansion;
   const char *DefinitionLanguage = "cpp";
   /// Access specifier for declarations inside class/struct/unions, empty for
   /// others.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -641,7 +641,8 @@
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
-HoverInfo getHoverContents(const DefinedMacro , ParsedAST ) {
+HoverInfo getHoverContents(const syntax::Token , const DefinedMacro ,
+   ParsedAST ) {
   HoverInfo HI;
   SourceManager  = AST.getSourceManager();
   HI.Name = std::string(Macro.Name);
@@ -670,6 +671,28 @@
 HI.Definition =
 ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))
 .str();
+
+  if (auto Expansion = 

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 439592.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  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
@@ -478,15 +478,46 @@
  HI.Definition = "Foo";
}},
 
-  // macro
+  // variable-like macro
+  {R"cpp(
+#define MACRO 41
+int x = [[MAC^RO]];
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+   }},
+
+  // function-like macro
   {R"cpp(
 // Best MACRO ever.
-#define MACRO(x,y,z) void foo(x, y, z);
+#define MACRO(x,y,z) void foo(x, y, z)
 [[MAC^RO]](int, double d, bool z = false);
 )cpp",
[](HoverInfo ) {
- HI.Name = "MACRO", HI.Kind = index::SymbolKind::Macro,
- HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z);";
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z)";
+ HI.MacroExpansion = "void foo(int, double d, bool z = false)";
+   }},
+
+  // nested macro
+  {R"cpp(
+#define STRINGIFY_AUX(s) #s
+#define STRINGIFY(s) STRINGIFY_AUX(s)
+#define DECL_STR(NAME, VALUE) const char *v_##NAME = STRINGIFY(VALUE)
+#define FOO 41
+
+[[DECL^_STR]](foo, FOO);
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "DECL_STR";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
+ "STRINGIFY(VALUE)";
+ HI.MacroExpansion = "const char *v_foo = \"41\"";
}},
 
   // constexprs
@@ -1070,6 +1101,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
@@ -1567,6 +1599,7 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+HI.MacroExpansion = "0";
   }},
   {
   R"cpp(// Macro
@@ -1577,6 +1610,8 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+// FIXME: expansion of MACRO isn't available in macro
+// definition/arguments
   }},
   {
   R"cpp(// Macro
@@ -1591,6 +1626,7 @@
 HI.Definition =
 R"cpp(#define MACRO  \
   { return 0; })cpp";
+HI.MacroExpansion = "{ return 0; }";
   }},
   {
   R"cpp(// Forward class declaration
@@ -2625,6 +2661,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
Index: clang-tools-extra/clangd/Hover.h
===
--- clang-tools-extra/clangd/Hover.h
+++ clang-tools-extra/clangd/Hover.h
@@ -71,6 +71,7 @@
   std::string Documentation;
   /// Source code containing the definition of the symbol.
   std::string Definition;
+  std::string MacroExpansion;
   const char *DefinitionLanguage = "cpp";
   /// Access specifier for declarations inside class/struct/unions, empty for
   /// others.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -641,7 +641,8 @@
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
-HoverInfo getHoverContents(const DefinedMacro , ParsedAST ) {
+HoverInfo getHoverContents(const syntax::Token , const DefinedMacro ,
+   ParsedAST ) {
   HoverInfo HI;
   SourceManager  = AST.getSourceManager();
   HI.Name = std::string(Macro.Name);
@@ -670,6 +671,29 @@
 HI.Definition =
 ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))
 .str();
+
+  auto Expansion = 

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 439591.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  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
@@ -478,15 +478,46 @@
  HI.Definition = "Foo";
}},
 
-  // macro
+  // variable-like macro
+  {R"cpp(
+#define MACRO 41
+int x = [[MAC^RO]];
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+   }},
+
+  // function-like macro
   {R"cpp(
 // Best MACRO ever.
-#define MACRO(x,y,z) void foo(x, y, z);
+#define MACRO(x,y,z) void foo(x, y, z)
 [[MAC^RO]](int, double d, bool z = false);
 )cpp",
[](HoverInfo ) {
- HI.Name = "MACRO", HI.Kind = index::SymbolKind::Macro,
- HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z);";
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z)";
+ HI.MacroExpansion = "void foo(int, double d, bool z = false)";
+   }},
+
+  // nested macro
+  {R"cpp(
+#define STRINGIFY_AUX(s) #s
+#define STRINGIFY(s) STRINGIFY_AUX(s)
+#define DECL_STR(NAME, VALUE) const char *v_##NAME = STRINGIFY(VALUE)
+#define FOO 41
+
+[[DECL^_STR]](foo, FOO);
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "DECL_STR";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
+ "STRINGIFY(VALUE)";
+ HI.MacroExpansion = "const char *v_foo = \"41\"";
}},
 
   // constexprs
@@ -1070,6 +1101,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
@@ -1567,6 +1599,7 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+HI.MacroExpansion = "0";
   }},
   {
   R"cpp(// Macro
@@ -1577,6 +1610,8 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+// FIXME: expansion of MACRO isn't available in macro
+// definition/arguments
   }},
   {
   R"cpp(// Macro
@@ -1591,6 +1626,7 @@
 HI.Definition =
 R"cpp(#define MACRO  \
   { return 0; })cpp";
+HI.MacroExpansion = "{ return 0; }";
   }},
   {
   R"cpp(// Forward class declaration
@@ -2625,6 +2661,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
Index: clang-tools-extra/clangd/Hover.h
===
--- clang-tools-extra/clangd/Hover.h
+++ clang-tools-extra/clangd/Hover.h
@@ -71,6 +71,7 @@
   std::string Documentation;
   /// Source code containing the definition of the symbol.
   std::string Definition;
+  std::string MacroExpansion;
   const char *DefinitionLanguage = "cpp";
   /// Access specifier for declarations inside class/struct/unions, empty for
   /// others.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -641,7 +641,8 @@
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
-HoverInfo getHoverContents(const DefinedMacro , ParsedAST ) {
+HoverInfo getHoverContents(const syntax::Token , const DefinedMacro ,
+   ParsedAST ) {
   HoverInfo HI;
   SourceManager  = AST.getSourceManager();
   HI.Name = std::string(Macro.Name);
@@ -670,6 +671,29 @@
 HI.Definition =
 ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))
 .str();
+
+  auto Expansion = 

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 439590.
daiyousei-qz added a comment.

use a rebased patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  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
@@ -478,15 +478,46 @@
  HI.Definition = "Foo";
}},
 
-  // macro
+  // variable-like macro
+  {R"cpp(
+#define MACRO 41
+int x = [[MAC^RO]];
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+   }},
+
+  // function-like macro
   {R"cpp(
 // Best MACRO ever.
-#define MACRO(x,y,z) void foo(x, y, z);
+#define MACRO(x,y,z) void foo(x, y, z)
 [[MAC^RO]](int, double d, bool z = false);
 )cpp",
[](HoverInfo ) {
- HI.Name = "MACRO", HI.Kind = index::SymbolKind::Macro,
- HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z);";
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z)";
+ HI.MacroExpansion = "void foo(int, double d, bool z = false)";
+   }},
+
+  // nested macro
+  {R"cpp(
+#define STRINGIFY_AUX(s) #s
+#define STRINGIFY(s) STRINGIFY_AUX(s)
+#define DECL_STR(NAME, VALUE) const char *v_##NAME = STRINGIFY(VALUE)
+#define FOO 41
+
+[[DECL^_STR]](foo, FOO);
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "DECL_STR";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
+ "STRINGIFY(VALUE)";
+ HI.MacroExpansion = "const char *v_foo = \"41\"";
}},
 
   // constexprs
@@ -1070,6 +1101,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
@@ -1567,6 +1599,7 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+HI.MacroExpansion = "0";
   }},
   {
   R"cpp(// Macro
@@ -1577,6 +1610,8 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+// FIXME: expansion of MACRO isn't available in macro
+// definition/arguments
   }},
   {
   R"cpp(// Macro
@@ -1591,6 +1626,7 @@
 HI.Definition =
 R"cpp(#define MACRO  \
   { return 0; })cpp";
+HI.MacroExpansion = "{ return 0; }";
   }},
   {
   R"cpp(// Forward class declaration
@@ -2625,6 +2661,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
Index: clang-tools-extra/clangd/Hover.h
===
--- clang-tools-extra/clangd/Hover.h
+++ clang-tools-extra/clangd/Hover.h
@@ -71,6 +71,7 @@
   std::string Documentation;
   /// Source code containing the definition of the symbol.
   std::string Definition;
+  std::string MacroExpansion;
   const char *DefinitionLanguage = "cpp";
   /// Access specifier for declarations inside class/struct/unions, empty for
   /// others.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -643,7 +643,8 @@
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
-HoverInfo getHoverContents(const DefinedMacro , ParsedAST ) {
+HoverInfo getHoverContents(const syntax::Token , const DefinedMacro ,
+   ParsedAST ) {
   HoverInfo HI;
   SourceManager  = AST.getSourceManager();
   HI.Name = std::string(Macro.Name);
@@ -672,6 +673,29 @@
 HI.Definition =
 ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))
   

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Sorry I'm a little occupied lately.

Regarding to the order of `Definition` and `MacroExpansion`, I experimented 
putting expansion first and noticed the following:

1. Short definition and short expansion: order doesn't matter, it's always clear
2. Short definition and long expansion: putting definition first may have 
advantage in providing a general overview about the macro
3. Long definition and long expansion: hover card isn't a great place to read 
even if it's scrollable. I'd rather jump to definition or use refactor feature 
to expand the macro in place :(

Therefore, I still put definition in front of the expansion in this update.

Regarding to the size limit of expansion:
Here I'm using a 2048 bytes buffer. If exceeded, no macro expansion will be 
displayed. It might be hard to limit size in term of lines because the expanded 
segment of code will be reformated.


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

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 439585.
daiyousei-qz added a comment.

- add context to patch
- use a fixed size buffer for expansion text


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  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
@@ -478,15 +478,46 @@
  HI.Definition = "Foo";
}},
 
-  // macro
+  // variable-like macro
+  {R"cpp(
+#define MACRO 41
+int x = [[MAC^RO]];
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+   }},
+
+  // function-like macro
   {R"cpp(
 // Best MACRO ever.
-#define MACRO(x,y,z) void foo(x, y, z);
+#define MACRO(x,y,z) void foo(x, y, z)
 [[MAC^RO]](int, double d, bool z = false);
 )cpp",
[](HoverInfo ) {
- HI.Name = "MACRO", HI.Kind = index::SymbolKind::Macro,
- HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z);";
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z)";
+ HI.MacroExpansion = "void foo(int, double d, bool z = false)";
+   }},
+
+  // nested macro
+  {R"cpp(
+#define STRINGIFY_AUX(s) #s
+#define STRINGIFY(s) STRINGIFY_AUX(s)
+#define DECL_STR(NAME, VALUE) const char *v_##NAME = STRINGIFY(VALUE)
+#define FOO 41
+
+[[DECL^_STR]](foo, FOO);
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "DECL_STR";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
+ "STRINGIFY(VALUE)";
+ HI.MacroExpansion = "const char *v_foo = \"41\"";
}},
 
   // constexprs
@@ -1070,6 +1101,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
@@ -1567,6 +1599,7 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+HI.MacroExpansion = "0";
   }},
   {
   R"cpp(// Macro
@@ -1577,6 +1610,8 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+// FIXME: expansion of MACRO isn't available in macro
+// definition/arguments
   }},
   {
   R"cpp(// Macro
@@ -1591,6 +1626,7 @@
 HI.Definition =
 R"cpp(#define MACRO  \
   { return 0; })cpp";
+HI.MacroExpansion = "{ return 0; }";
   }},
   {
   R"cpp(// Forward class declaration
@@ -2625,6 +2661,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
Index: clang-tools-extra/clangd/Hover.h
===
--- clang-tools-extra/clangd/Hover.h
+++ clang-tools-extra/clangd/Hover.h
@@ -71,6 +71,7 @@
   std::string Documentation;
   /// Source code containing the definition of the symbol.
   std::string Definition;
+  std::string MacroExpansion;
   const char *DefinitionLanguage = "cpp";
   /// Access specifier for declarations inside class/struct/unions, empty for
   /// others.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -643,7 +643,8 @@
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
-HoverInfo getHoverContents(const DefinedMacro , ParsedAST ) {
+HoverInfo getHoverContents(const syntax::Token , const DefinedMacro ,
+   ParsedAST ) {
   HoverInfo HI;
   SourceManager  = AST.getSourceManager();
   HI.Name = std::string(Macro.Name);
@@ -672,6 +673,29 @@
 HI.Definition =
 ("#define " + 

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

To elaborate on known situation where expansion isn't available:

1. expansion doesn't work on site of #define
2. expansion doesn't work for _Pragma(...) and alike
3. expansion doesn't work with builtin macro
4. expansion doesn't work for macros as arguments of function-like macro

Basically, anywhere the same code action isn't available. Need this be 
documented somewhere?


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

[PATCH] D125863: [clangd] Dont mark terminating PP-directives as skipped

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

I have a similar change recently presented in 
https://github.com/clangd/clangd/issues/1181 that makes all conditional 
preprocessor lines active. Please refer to the change if that's helpful :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125863

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

[PATCH] D126757: [AST] Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Yes, please do so. I'm using the web interface since the arc tool isn't very 
friendly to Windows user :(

Name: Qingyuan Zheng
Email: qyzhe...@outlook.com


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126757

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

[PATCH] D127082: Add Macro Expansion to Hover

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz created this revision.
daiyousei-qz added reviewers: nridge, sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman.
Herald added a project: All.
daiyousei-qz requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

This patch adds macro expansion preview to hover info. Basically, the refactor 
infrastructure for expanding macro is used for this purpose. The following 
steps are added to getHoverContents for macros:

1. calling AST.getTokens().expansionStartingAt(...) to get expanded tokens
2. calling reformat(...) to format expanded tokens

Some opinions are wanted:

1. Should we present macro expansion before definition in the hover card?
2. Should we truncate/ignore macro expansion if it's too long?

Also, some limitation applies:

1. Expansion isn't available in macro definition/arguments as the refactor code 
action isn't either.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127082

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  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
@@ -478,15 +478,46 @@
  HI.Definition = "Foo";
}},
 
-  // macro
+  // variable-like macro
+  {R"cpp(
+#define MACRO 41
+int x = [[MAC^RO]];
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+   }},
+
+  // function-like macro
   {R"cpp(
 // Best MACRO ever.
-#define MACRO(x,y,z) void foo(x, y, z);
+#define MACRO(x,y,z) void foo(x, y, z)
 [[MAC^RO]](int, double d, bool z = false);
 )cpp",
[](HoverInfo ) {
- HI.Name = "MACRO", HI.Kind = index::SymbolKind::Macro,
- HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z);";
+ HI.Name = "MACRO";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define MACRO(x, y, z) void foo(x, y, z)";
+ HI.MacroExpansion = "void foo(int, double d, bool z = false)";
+   }},
+
+  // nested macro
+  {R"cpp(
+#define STRINGIFY_AUX(s) #s
+#define STRINGIFY(s) STRINGIFY_AUX(s)
+#define DECL_STR(NAME, VALUE) const char *v_##NAME = STRINGIFY(VALUE)
+#define FOO 41
+
+[[DECL^_STR]](foo, FOO);
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "DECL_STR";
+ HI.Kind = index::SymbolKind::Macro;
+ HI.Definition = "#define DECL_STR(NAME, VALUE) const char *v_##NAME = "
+ "STRINGIFY(VALUE)";
+ HI.MacroExpansion = "const char *v_foo = \"41\"";
}},
 
   // constexprs
@@ -1070,6 +1101,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
@@ -1567,6 +1599,7 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+HI.MacroExpansion = "0";
   }},
   {
   R"cpp(// Macro
@@ -1577,6 +1610,8 @@
 HI.Name = "MACRO";
 HI.Kind = index::SymbolKind::Macro;
 HI.Definition = "#define MACRO 0";
+// FIXME: expansion of MACRO isn't available in macro
+// definition/arguments
   }},
   {
   R"cpp(// Macro
@@ -1591,6 +1626,7 @@
 HI.Definition =
 R"cpp(#define MACRO  \
   { return 0; })cpp";
+HI.MacroExpansion = "{ return 0; }";
   }},
   {
   R"cpp(// Forward class declaration
@@ -2625,6 +2661,7 @@
 EXPECT_EQ(H->Kind, Expected.Kind);
 EXPECT_EQ(H->Documentation, Expected.Documentation);
 EXPECT_EQ(H->Definition, Expected.Definition);
+EXPECT_EQ(H->MacroExpansion, Expected.MacroExpansion);
 EXPECT_EQ(H->Type, Expected.Type);
 EXPECT_EQ(H->ReturnType, Expected.ReturnType);
 EXPECT_EQ(H->Parameters, Expected.Parameters);
Index: clang-tools-extra/clangd/Hover.h
===
--- clang-tools-extra/clangd/Hover.h
+++ clang-tools-extra/clangd/Hover.h
@@ -71,6 +71,7 @@
   std::string Documentation;
   /// Source code containing the definition of the symbol.
   std::string Definition;
+  std::string MacroExpansion;
   const char *DefinitionLanguage = "cpp";
   /// 

[PATCH] D126757: Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 434196.
daiyousei-qz marked an inline comment as done.
daiyousei-qz added a comment.

squashed commits.
updated comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126757

Files:
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
  clang/include/clang/AST/RecursiveASTVisitor.h


Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -2021,7 +2021,7 @@
 
 DEF_TRAVERSE_DECL(CXXRecordDecl, { TRY_TO(TraverseCXXRecordHelper(D)); })
 
-#define DEF_TRAVERSE_TMPL_SPEC_DECL(TMPLDECLKIND)  
\
+#define DEF_TRAVERSE_TMPL_SPEC_DECL(TMPLDECLKIND, DECLKIND)
\
   DEF_TRAVERSE_DECL(TMPLDECLKIND##TemplateSpecializationDecl, {
\
 /* For implicit instantiations ("set x;"), we don't want to   
\
recurse at all, since the instatiated template isn't written in 
\
@@ -2034,18 +2034,23 @@
 if (TypeSourceInfo *TSI = D->getTypeAsWritten())   
\
   TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));  
\

\
-TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));  
\
-if (!getDerived().shouldVisitTemplateInstantiations() &&   
\
-D->getTemplateSpecializationKind() != TSK_ExplicitSpecialization)  
\
+if (getDerived().shouldVisitTemplateInstantiations() ||
\
+D->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) {
\
+  /* Traverse base definition for explicit specializations */  
\
+  TRY_TO(Traverse##DECLKIND##Helper(D));   
\
+} else {   
\
+  TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));
\
+   
\
   /* Returning from here skips traversing the  
\
  declaration context of the *TemplateSpecializationDecl
\
  (embedded in the DEF_TRAVERSE_DECL() macro)   
\
  which contains the instantiated members of the template. */   
\
   return true; 
\
+}  
\
   })
 
-DEF_TRAVERSE_TMPL_SPEC_DECL(Class)
-DEF_TRAVERSE_TMPL_SPEC_DECL(Var)
+DEF_TRAVERSE_TMPL_SPEC_DECL(Class, CXXRecord)
+DEF_TRAVERSE_TMPL_SPEC_DECL(Var, Var)
 
 template 
 bool RecursiveASTVisitor::TraverseTemplateArgumentLocsHelper(
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -807,6 +807,19 @@
 $Function_deprecated[[Foo]]($Parameter[[x]]); 
 $Function_deprecated[[Foo]]($Parameter[[x]]); 
 }
+  )cpp",
+  // Explicit template specialization
+  R"cpp(
+struct $Class_decl[[Base]]{};
+template 
+struct $Class_decl[[S]] : public $Class[[Base]] {};
+template <> 
+struct $Class_decl[[S]] : public $Class[[Base]] {};
+
+template 
+$TemplateParameter[[T]] $Variable_decl[[x]] = {};
+template <>
+int $Variable_decl[[x]] = (int)sizeof($Class[[Base]]);
   )cpp"};
   for (const auto  : TestCases)
 // Mask off scope modifiers to keep the tests manageable.


Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -2021,7 +2021,7 @@
 
 DEF_TRAVERSE_DECL(CXXRecordDecl, { TRY_TO(TraverseCXXRecordHelper(D)); })
 
-#define DEF_TRAVERSE_TMPL_SPEC_DECL(TMPLDECLKIND)  \
+#define DEF_TRAVERSE_TMPL_SPEC_DECL(TMPLDECLKIND, DECLKIND)\
   DEF_TRAVERSE_DECL(TMPLDECLKIND##TemplateSpecializationDecl, {\
 /* For implicit instantiations ("set x;"), we don't want to   \
recurse at all, since the instatiated template isn't written in \
@@ -2034,18 +2034,23 @@
 if (TypeSourceInfo *TSI = D->getTypeAsWritten())   \
   TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));  \
\
-

[PATCH] D126757: Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 433590.
daiyousei-qz added a comment.
Herald added a subscriber: arphaman.
Herald added a project: clang-tools-extra.

added unit test
moved redundant visit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126757

Files:
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
  clang/include/clang/AST/RecursiveASTVisitor.h


Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -2034,12 +2034,13 @@
 if (TypeSourceInfo *TSI = D->getTypeAsWritten())   
\
   TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));  
\

\
-TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));  
\
 if (getDerived().shouldVisitTemplateInstantiations() ||
\
 D->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) {
\
   /* Traverse base definition for explicit specializations */  
\
   TRY_TO(Traverse##DECLKIND##Helper(D));   
\
 } else {   
\
+  TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));
\
+   
\
   /* Returning from here skips traversing the  
\
  declaration context of the *TemplateSpecializationDecl
\
  (embedded in the DEF_TRAVERSE_DECL() macro)   
\
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -807,6 +807,19 @@
 $Function_deprecated[[Foo]]($Parameter[[x]]); 
 $Function_deprecated[[Foo]]($Parameter[[x]]); 
 }
+  )cpp",
+  // Template specialization
+  R"cpp(
+struct $Class_decl[[Base]]{};
+template 
+struct $Class_decl[[S]] : public $Class[[Base]] {};
+template <> 
+struct $Class_decl[[S]] : public $Class[[Base]] {};
+
+template 
+$TemplateParameter[[T]] $Variable_decl[[x]] = {};
+template <>
+int $Variable_decl[[x]] = (int)sizeof($Class[[Base]]);
   )cpp"};
   for (const auto  : TestCases)
 // Mask off scope modifiers to keep the tests manageable.


Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -2034,12 +2034,13 @@
 if (TypeSourceInfo *TSI = D->getTypeAsWritten())   \
   TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));  \
\
-TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));  \
 if (getDerived().shouldVisitTemplateInstantiations() ||\
 D->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) {\
   /* Traverse base definition for explicit specializations */  \
   TRY_TO(Traverse##DECLKIND##Helper(D));   \
 } else {   \
+  TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));\
+   \
   /* Returning from here skips traversing the  \
  declaration context of the *TemplateSpecializationDecl\
  (embedded in the DEF_TRAVERSE_DECL() macro)   \
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -807,6 +807,19 @@
 $Function_deprecated[[Foo]]($Parameter[[x]]); 
 $Function_deprecated[[Foo]]($Parameter[[x]]); 
 }
+  )cpp",
+  // Template specialization
+  R"cpp(
+struct $Class_decl[[Base]]{};
+template 
+struct $Class_decl[[S]] : public $Class[[Base]] {};
+template <> 
+struct $Class_decl[[S]] : public $Class[[Base]] {};
+
+template 
+$TemplateParameter[[T]] 

[PATCH] D126757: Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz created this revision.
daiyousei-qz added reviewers: nridge, klimek.
Herald added subscribers: usaxena95, kadircet.
Herald added a project: All.
daiyousei-qz requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added a project: clang.

The original discussion starts at this issue: 
https://github.com/clangd/clangd/issues/1132
where clangd's semantic highlighting is missing for symbols of a template 
specialization definition. It turns out the visitor didn't traverse the base 
classes of Class/Var##TemplateSpecializationDecl, i.e. CXXRecordDecl/VarDecl. 
This patch adds them back as what is done in DEF_TRAVERSE_TMPL_PART_SPEC_DECL.

I'm not sure who should be included as reviewers. Please edit as needed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126757

Files:
  clang/include/clang/AST/RecursiveASTVisitor.h


Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -2021,7 +2021,7 @@
 
 DEF_TRAVERSE_DECL(CXXRecordDecl, { TRY_TO(TraverseCXXRecordHelper(D)); })
 
-#define DEF_TRAVERSE_TMPL_SPEC_DECL(TMPLDECLKIND)  
\
+#define DEF_TRAVERSE_TMPL_SPEC_DECL(TMPLDECLKIND, DECLKIND)
\
   DEF_TRAVERSE_DECL(TMPLDECLKIND##TemplateSpecializationDecl, {
\
 /* For implicit instantiations ("set x;"), we don't want to   
\
recurse at all, since the instatiated template isn't written in 
\
@@ -2035,17 +2035,21 @@
   TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));  
\

\
 TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));  
\
-if (!getDerived().shouldVisitTemplateInstantiations() &&   
\
-D->getTemplateSpecializationKind() != TSK_ExplicitSpecialization)  
\
+if (getDerived().shouldVisitTemplateInstantiations() ||
\
+D->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) {
\
+  /* Traverse base definition for explicit specializations */  
\
+  TRY_TO(Traverse##DECLKIND##Helper(D));   
\
+} else {   
\
   /* Returning from here skips traversing the  
\
  declaration context of the *TemplateSpecializationDecl
\
  (embedded in the DEF_TRAVERSE_DECL() macro)   
\
  which contains the instantiated members of the template. */   
\
   return true; 
\
+}  
\
   })
 
-DEF_TRAVERSE_TMPL_SPEC_DECL(Class)
-DEF_TRAVERSE_TMPL_SPEC_DECL(Var)
+DEF_TRAVERSE_TMPL_SPEC_DECL(Class, CXXRecord)
+DEF_TRAVERSE_TMPL_SPEC_DECL(Var, Var)
 
 template 
 bool RecursiveASTVisitor::TraverseTemplateArgumentLocsHelper(


Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -2021,7 +2021,7 @@
 
 DEF_TRAVERSE_DECL(CXXRecordDecl, { TRY_TO(TraverseCXXRecordHelper(D)); })
 
-#define DEF_TRAVERSE_TMPL_SPEC_DECL(TMPLDECLKIND)  \
+#define DEF_TRAVERSE_TMPL_SPEC_DECL(TMPLDECLKIND, DECLKIND)\
   DEF_TRAVERSE_DECL(TMPLDECLKIND##TemplateSpecializationDecl, {\
 /* For implicit instantiations ("set x;"), we don't want to   \
recurse at all, since the instatiated template isn't written in \
@@ -2035,17 +2035,21 @@
   TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));  \
\
 TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));  \
-if (!getDerived().shouldVisitTemplateInstantiations() &&   \
-D->getTemplateSpecializationKind() != TSK_ExplicitSpecialization)  \
+if (getDerived().shouldVisitTemplateInstantiations() ||\
+D->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) {\
+  /* Traverse base definition for explicit specializations */  \
+  TRY_TO(Traverse##DECLKIND##Helper(D));   \
+} else {   \
   /* Returning from here skips traversing the  \
  declaration context of the *TemplateSpecializationDecl\
  (embedded in the