[PATCH] D76098: [clangd] Do not trigger go-to-def textual fallback inside string literals
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb89202e842ac: [clangd] Do not trigger go-to-def textual
fallback inside string literals (authored by nridge).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76098/new/
https://reviews.llvm.org/D76098
Files:
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -644,7 +644,8 @@
// Comment mentioning M^yClass
)cpp",
R"cpp(// String
-struct [[MyClass]] {};
+struct MyClass {};
+// Not triggered for string literal tokens.
const char* s = "String literal mentioning M^yClass";
)cpp",
R"cpp(// Ifdef'ed out code
@@ -696,7 +697,7 @@
EXPECT_EQ(Results[0].PreferredDeclaration.range, *WantDecl) << Test;
}
}
-}
+} // namespace
TEST(LocateSymbol, Ambiguous) {
auto T = Annotations(R"cpp(
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -374,6 +374,18 @@
unsigned WordOffset = Word.data() - Code.data();
SourceLocation WordStart = SM.getComposedLoc(File, WordOffset);
+ // Attempt to determine the kind of token that contains the word,
+ // and bail if it's a string literal. Note that we cannot always
+ // determine the token kind (e.g. comments, for which we do want
+ // to activate, are not retained by TokenBuffer).
+ for (syntax::Token T :
+ syntax::spelledTokensTouching(WordStart, AST.getTokens())) {
+if (T.range(AST.getSourceManager()).touches(WordOffset + Word.size())) {
+ if (isStringLiteral(T.kind()))
+return {};
+}
+ }
+
// Do not consider tokens that survived preprocessing.
// We are erring on the safe side here, as a user may expect to get
// accurate (as opposed to textual-heuristic) results for such tokens.
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -644,7 +644,8 @@
// Comment mentioning M^yClass
)cpp",
R"cpp(// String
-struct [[MyClass]] {};
+struct MyClass {};
+// Not triggered for string literal tokens.
const char* s = "String literal mentioning M^yClass";
)cpp",
R"cpp(// Ifdef'ed out code
@@ -696,7 +697,7 @@
EXPECT_EQ(Results[0].PreferredDeclaration.range, *WantDecl) << Test;
}
}
-}
+} // namespace
TEST(LocateSymbol, Ambiguous) {
auto T = Annotations(R"cpp(
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -374,6 +374,18 @@
unsigned WordOffset = Word.data() - Code.data();
SourceLocation WordStart = SM.getComposedLoc(File, WordOffset);
+ // Attempt to determine the kind of token that contains the word,
+ // and bail if it's a string literal. Note that we cannot always
+ // determine the token kind (e.g. comments, for which we do want
+ // to activate, are not retained by TokenBuffer).
+ for (syntax::Token T :
+ syntax::spelledTokensTouching(WordStart, AST.getTokens())) {
+if (T.range(AST.getSourceManager()).touches(WordOffset + Word.size())) {
+ if (isStringLiteral(T.kind()))
+return {};
+}
+ }
+
// Do not consider tokens that survived preprocessing.
// We are erring on the safe side here, as a user may expect to get
// accurate (as opposed to textual-heuristic) results for such tokens.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D76098: [clangd] Do not trigger go-to-def textual fallback inside string literals
nridge updated this revision to Diff 251476.
nridge added a comment.
Use WordStart
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76098/new/
https://reviews.llvm.org/D76098
Files:
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -628,7 +628,8 @@
// Comment mentioning M^yClass
)cpp",
R"cpp(// String
-struct [[MyClass]] {};
+struct MyClass {};
+// Not triggered for string literal tokens.
const char* s = "String literal mentioning M^yClass";
)cpp",
R"cpp(// Ifdef'ed out code
@@ -680,7 +681,7 @@
EXPECT_EQ(Results[0].PreferredDeclaration.range, *WantDecl) << Test;
}
}
-}
+} // namespace
TEST(LocateSymbol, Ambiguous) {
auto T = Annotations(R"cpp(
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -374,6 +374,18 @@
unsigned WordOffset = Word.data() - Code.data();
SourceLocation WordStart = SM.getComposedLoc(File, WordOffset);
+ // Attempt to determine the kind of token that contains the word,
+ // and bail if it's a string literal. Note that we cannot always
+ // determine the token kind (e.g. comments, for which we do want
+ // to activate, are not retained by TokenBuffer).
+ for (syntax::Token T :
+ syntax::spelledTokensTouching(WordStart, AST.getTokens())) {
+if (T.range(AST.getSourceManager()).touches(WordOffset + Word.size())) {
+ if (isStringLiteral(T.kind()))
+return {};
+}
+ }
+
// Do not consider tokens that survived preprocessing.
// We are erring on the safe side here, as a user may expect to get
// accurate (as opposed to textual-heuristic) results for such tokens.
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -628,7 +628,8 @@
// Comment mentioning M^yClass
)cpp",
R"cpp(// String
-struct [[MyClass]] {};
+struct MyClass {};
+// Not triggered for string literal tokens.
const char* s = "String literal mentioning M^yClass";
)cpp",
R"cpp(// Ifdef'ed out code
@@ -680,7 +681,7 @@
EXPECT_EQ(Results[0].PreferredDeclaration.range, *WantDecl) << Test;
}
}
-}
+} // namespace
TEST(LocateSymbol, Ambiguous) {
auto T = Annotations(R"cpp(
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -374,6 +374,18 @@
unsigned WordOffset = Word.data() - Code.data();
SourceLocation WordStart = SM.getComposedLoc(File, WordOffset);
+ // Attempt to determine the kind of token that contains the word,
+ // and bail if it's a string literal. Note that we cannot always
+ // determine the token kind (e.g. comments, for which we do want
+ // to activate, are not retained by TokenBuffer).
+ for (syntax::Token T :
+ syntax::spelledTokensTouching(WordStart, AST.getTokens())) {
+if (T.range(AST.getSourceManager()).touches(WordOffset + Word.size())) {
+ if (isStringLiteral(T.kind()))
+return {};
+}
+ }
+
// Do not consider tokens that survived preprocessing.
// We are erring on the safe side here, as a user may expect to get
// accurate (as opposed to textual-heuristic) results for such tokens.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D76098: [clangd] Do not trigger go-to-def textual fallback inside string literals
nridge marked an inline comment as done.
nridge added inline comments.
Comment at: clang-tools-extra/clangd/XRefs.cpp:381
+ // to activate, are not retained by TokenBuffer).
+ for (syntax::Token T : syntax::spelledTokensTouching(Loc, AST.getTokens())) {
+if (T.range(AST.getSourceManager()).touches(WordOffset + Word.size())) {
Whoops, meant to use `WordStart` rather than `Loc` here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76098/new/
https://reviews.llvm.org/D76098
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D76098: [clangd] Do not trigger go-to-def textual fallback inside string literals
nridge updated this revision to Diff 251418.
nridge added a comment.
Take a blacklisting approach
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76098/new/
https://reviews.llvm.org/D76098
Files:
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -628,7 +628,8 @@
// Comment mentioning M^yClass
)cpp",
R"cpp(// String
-struct [[MyClass]] {};
+struct MyClass {};
+// Not triggered for string literal tokens.
const char* s = "String literal mentioning M^yClass";
)cpp",
R"cpp(// Ifdef'ed out code
@@ -680,7 +681,7 @@
EXPECT_EQ(Results[0].PreferredDeclaration.range, *WantDecl) << Test;
}
}
-}
+} // namespace
TEST(LocateSymbol, Ambiguous) {
auto T = Annotations(R"cpp(
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -374,6 +374,17 @@
unsigned WordOffset = Word.data() - Code.data();
SourceLocation WordStart = SM.getComposedLoc(File, WordOffset);
+ // Attempt to determine the kind of token that contains the word,
+ // and bail if it's a string literal. Note that we cannot always
+ // determine the token kind (e.g. comments, for which we do want
+ // to activate, are not retained by TokenBuffer).
+ for (syntax::Token T : syntax::spelledTokensTouching(Loc, AST.getTokens())) {
+if (T.range(AST.getSourceManager()).touches(WordOffset + Word.size())) {
+ if (isStringLiteral(T.kind()))
+return {};
+}
+ }
+
// Do not consider tokens that survived preprocessing.
// We are erring on the safe side here, as a user may expect to get
// accurate (as opposed to textual-heuristic) results for such tokens.
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -628,7 +628,8 @@
// Comment mentioning M^yClass
)cpp",
R"cpp(// String
-struct [[MyClass]] {};
+struct MyClass {};
+// Not triggered for string literal tokens.
const char* s = "String literal mentioning M^yClass";
)cpp",
R"cpp(// Ifdef'ed out code
@@ -680,7 +681,7 @@
EXPECT_EQ(Results[0].PreferredDeclaration.range, *WantDecl) << Test;
}
}
-}
+} // namespace
TEST(LocateSymbol, Ambiguous) {
auto T = Annotations(R"cpp(
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -374,6 +374,17 @@
unsigned WordOffset = Word.data() - Code.data();
SourceLocation WordStart = SM.getComposedLoc(File, WordOffset);
+ // Attempt to determine the kind of token that contains the word,
+ // and bail if it's a string literal. Note that we cannot always
+ // determine the token kind (e.g. comments, for which we do want
+ // to activate, are not retained by TokenBuffer).
+ for (syntax::Token T : syntax::spelledTokensTouching(Loc, AST.getTokens())) {
+if (T.range(AST.getSourceManager()).touches(WordOffset + Word.size())) {
+ if (isStringLiteral(T.kind()))
+return {};
+}
+ }
+
// Do not consider tokens that survived preprocessing.
// We are erring on the safe side here, as a user may expect to get
// accurate (as opposed to textual-heuristic) results for such tokens.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D76098: [clangd] Do not trigger go-to-def textual fallback inside string literals
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/XRefs.cpp:367
+ auto Tokens = syntax::spelledTokensTouching(Loc, AST.getTokens());
+ if (Tokens.size() != 1)
+return {};
this means you're not going to resolve `foo` in `a.^foo` (you're touching two
tokens).
The cleanest thing seems to be to use the word you've identified: iterate over
the `spelledTokensTouching(WordStart)` and accept the one where
`tok.range(SM).touches(WordOffset + Word.size())`
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:343
SrcBuffer.data() + SrcBuffer.size());
+ L.SetCommentRetentionState(true);
Yikes, I didn't remember TokenBuffer doesn't currently record comment tokens.
I'm afraid this isn't a trivial change and would definitely need tests to
verify it doesn't interfere with translating between spelled/expanded tokens
(I'm pretty sure there are tests that comments *aren't* retained now in
TokensTest.cpp, part of SyntaxTests)
Given that, the shorter route for this patch would be to blacklist string
literals rather than whitelisting comments + identifiers.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76098/new/
https://reviews.llvm.org/D76098
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
