[clang-tools-extra] [clangd] Fix is spelled in source bug (PR #76668)
Andrew Schenk via cfe-commits
cfe-commits at lists.llvm.org
Sat Jan 27 05:20:09 PST 2024
https://github.com/schenka0 updated https://github.com/llvm/llvm-project/pull/76668
>From fd5e586d807fa4532f26188822ac5790202673bc Mon Sep 17 00:00:00 2001
From: schenka0 <154034018+schenka0 at users.noreply.github.com>
Date: Mon, 1 Jan 2024 01:31:05 -0500
Subject: [PATCH 1/4] Check for invalid SLocEntry before getting spelling
---
clang-tools-extra/clangd/SourceCode.cpp | 7 ++++++-
.../clangd/unittests/SourceCodeTests.cpp | 15 +++++++++++++++
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp
index 835038423fdf372..8c573cc6fc064ae 100644
--- a/clang-tools-extra/clangd/SourceCode.cpp
+++ b/clang-tools-extra/clangd/SourceCode.cpp
@@ -232,7 +232,12 @@ bool isSpelledInSource(SourceLocation Loc, const SourceManager &SM) {
if (Loc.isFileID())
return true;
auto Spelling = SM.getDecomposedSpellingLoc(Loc);
- StringRef SpellingFile = SM.getSLocEntry(Spelling.first).getFile().getName();
+ bool InvalidSLocEntry = false;
+ const auto SLocEntry = SM.getSLocEntry(Spelling.first, &InvalidSLocEntry);
+ if (InvalidSLocEntry) {
+ return false;
+ }
+ const StringRef SpellingFile = SLocEntry.getFile().getName();
if (SpellingFile == "<scratch space>")
return false;
if (SpellingFile == "<built-in>")
diff --git a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
index 08abde87df6d4d0..5dced4d317c6059 100644
--- a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -813,6 +813,21 @@ TEST(SourceCodeTests, isKeywords) {
EXPECT_FALSE(isKeyword("override", LangOpts));
}
+TEST(SourceCodeTests, isSpelledInSource) {
+ Annotations Test(R"cpp(
+ int abc = 1;
+ )cpp");
+
+ ParsedAST AST = TestTU::withCode(Test.code()).build();
+ llvm::errs() << Test.code();
+ const SourceManager &SM = AST.getSourceManager();
+
+ EXPECT_TRUE(
+ isSpelledInSource(SM.getLocForStartOfFile(SM.getMainFileID()), SM));
+ EXPECT_TRUE(isSpelledInSource(SourceLocation(), SM));
+ EXPECT_FALSE(isSpelledInSource(SourceLocation::getFromRawEncoding(-1), SM));
+}
+
struct IncrementalTestStep {
llvm::StringRef Src;
llvm::StringRef Contents;
>From 612c1fea8de7dfea67d5b4fdd23d6f13b9851607 Mon Sep 17 00:00:00 2001
From: schenka0 <154034018+schenka0 at users.noreply.github.com>
Date: Mon, 1 Jan 2024 11:49:24 -0500
Subject: [PATCH 2/4] Update clang-tools-extra/clangd/SourceCode.cpp
Co-authored-by: Younan Zhang <zyn7109 at gmail.com>
---
clang-tools-extra/clangd/SourceCode.cpp | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp
index 8c573cc6fc064ae..58a0d142777d000 100644
--- a/clang-tools-extra/clangd/SourceCode.cpp
+++ b/clang-tools-extra/clangd/SourceCode.cpp
@@ -234,9 +234,8 @@ bool isSpelledInSource(SourceLocation Loc, const SourceManager &SM) {
auto Spelling = SM.getDecomposedSpellingLoc(Loc);
bool InvalidSLocEntry = false;
const auto SLocEntry = SM.getSLocEntry(Spelling.first, &InvalidSLocEntry);
- if (InvalidSLocEntry) {
+ if (InvalidSLocEntry)
return false;
- }
const StringRef SpellingFile = SLocEntry.getFile().getName();
if (SpellingFile == "<scratch space>")
return false;
>From d05e29bb46e2d19317fc667ba37e4549e551496a Mon Sep 17 00:00:00 2001
From: schenka0 <154034018+schenka0 at users.noreply.github.com>
Date: Mon, 1 Jan 2024 21:49:59 -0500
Subject: [PATCH 3/4] Update invalid SourceLocation in testpoint. Use a value
with the MSB set to 1 (so it is not a FileID) and the remaining bits 0
Address review feedback on formatting and const
---
clang-tools-extra/clangd/SourceCode.cpp | 2 +-
clang-tools-extra/clangd/unittests/SourceCodeTests.cpp | 9 +++------
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp
index 58a0d142777d000..3e741f6e0b536ba 100644
--- a/clang-tools-extra/clangd/SourceCode.cpp
+++ b/clang-tools-extra/clangd/SourceCode.cpp
@@ -236,7 +236,7 @@ bool isSpelledInSource(SourceLocation Loc, const SourceManager &SM) {
const auto SLocEntry = SM.getSLocEntry(Spelling.first, &InvalidSLocEntry);
if (InvalidSLocEntry)
return false;
- const StringRef SpellingFile = SLocEntry.getFile().getName();
+ StringRef SpellingFile = SLocEntry.getFile().getName();
if (SpellingFile == "<scratch space>")
return false;
if (SpellingFile == "<built-in>")
diff --git a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
index 5dced4d317c6059..a85935125789972 100644
--- a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -814,18 +814,15 @@ TEST(SourceCodeTests, isKeywords) {
}
TEST(SourceCodeTests, isSpelledInSource) {
- Annotations Test(R"cpp(
- int abc = 1;
- )cpp");
-
+ Annotations Test("");
ParsedAST AST = TestTU::withCode(Test.code()).build();
- llvm::errs() << Test.code();
const SourceManager &SM = AST.getSourceManager();
EXPECT_TRUE(
isSpelledInSource(SM.getLocForStartOfFile(SM.getMainFileID()), SM));
EXPECT_TRUE(isSpelledInSource(SourceLocation(), SM));
- EXPECT_FALSE(isSpelledInSource(SourceLocation::getFromRawEncoding(-1), SM));
+ EXPECT_FALSE(isSpelledInSource(
+ SourceLocation::getFromRawEncoding(SourceLocation::UIntTy(1 << 31)), SM));
}
struct IncrementalTestStep {
>From 59ce9cd041a46cf25d623074ef2555c7a4f0c245 Mon Sep 17 00:00:00 2001
From: schenka0 <154034018+schenka0 at users.noreply.github.com>
Date: Sat, 27 Jan 2024 08:19:51 -0500
Subject: [PATCH 4/4] Address review feedback. Add test comment
---
clang-tools-extra/clangd/unittests/SourceCodeTests.cpp | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
index a85935125789972..1be5b7f6a8dbbad 100644
--- a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -820,6 +820,13 @@ TEST(SourceCodeTests, isSpelledInSource) {
EXPECT_TRUE(
isSpelledInSource(SM.getLocForStartOfFile(SM.getMainFileID()), SM));
+
+ // Check that isSpelledInSource() handles various invalid source locations
+ // gracefully.
+ //
+ // Returning true for SourceLocation() is a behavior that falls out of the
+ // current implementation, which has an early exit for isFileID().
+ // FIXME: Should it return false on SourceLocation()? Does it matter?
EXPECT_TRUE(isSpelledInSource(SourceLocation(), SM));
EXPECT_FALSE(isSpelledInSource(
SourceLocation::getFromRawEncoding(SourceLocation::UIntTy(1 << 31)), SM));
More information about the cfe-commits
mailing list