[PATCH] D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include
This revision was automatically updated to reflect the committed changes.
Closed by commit rL370029: [clangd] Fix toHalfOpenFileRange where start/end
endpoints are in different… (authored by sammccall, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D66590?vs=216609=217325#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66590/new/
https://reviews.llvm.org/D66590
Files:
clang-tools-extra/trunk/clangd/SourceCode.cpp
clang-tools-extra/trunk/clangd/SourceCode.h
clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp
Index: clang-tools-extra/trunk/clangd/SourceCode.h
===
--- clang-tools-extra/trunk/clangd/SourceCode.h
+++ clang-tools-extra/trunk/clangd/SourceCode.h
@@ -83,6 +83,11 @@
/// the main file.
bool isInsideMainFile(SourceLocation Loc, const SourceManager );
+/// Returns the #include location through which IncludedFIle was loaded.
+/// Where SM.getIncludeLoc() returns the location of the *filename*, which may
+/// be in a macro, includeHashLoc() returns the location of the #.
+SourceLocation includeHashLoc(FileID IncludedFile, const SourceManager );
+
/// Returns true if the token at Loc is spelled in the source code.
/// This is not the case for:
/// * symbols formed via macro concatenation, the spelling location will
Index: clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp
@@ -11,9 +11,11 @@
#include "SourceCode.h"
#include "TestTU.h"
#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
#include "clang/Format/Format.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Testing/Support/Annotations.h"
#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -505,6 +507,53 @@
CheckRange("f");
}
+TEST(SourceCodeTests, HalfOpenFileRangePathologicalPreprocessor) {
+ const char *Case = R"cpp(
+#define MACRO while(1)
+void test() {
+[[#include "Expand.inc"
+br^eak]];
+}
+ )cpp";
+ Annotations Test(Case);
+ auto TU = TestTU::withCode(Test.code());
+ TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
+ auto AST = TU.build();
+
+ const auto = cast(findDecl(AST, "test"));
+ const auto = cast(Func.getBody());
+ const auto = cast(*Body->child_begin());
+ llvm::Optional Range = toHalfOpenFileRange(
+ AST.getSourceManager(), AST.getASTContext().getLangOpts(),
+ Loop->getSourceRange());
+ ASSERT_TRUE(Range) << "Failed to get file range";
+ EXPECT_EQ(AST.getSourceManager().getFileOffset(Range->getBegin()),
+Test.llvm::Annotations::range().Begin);
+ EXPECT_EQ(AST.getSourceManager().getFileOffset(Range->getEnd()),
+Test.llvm::Annotations::range().End);
+}
+
+TEST(SourceCodeTests, IncludeHashLoc) {
+ const char *Case = R"cpp(
+$foo^#include "foo.inc"
+#define HEADER "bar.inc"
+ $bar^# include HEADER
+ )cpp";
+ Annotations Test(Case);
+ auto TU = TestTU::withCode(Test.code());
+ TU.AdditionalFiles["foo.inc"] = "int foo;\n";
+ TU.AdditionalFiles["bar.inc"] = "int bar;\n";
+ auto AST = TU.build();
+ const auto& SM = AST.getSourceManager();
+
+ FileID Foo = SM.getFileID(findDecl(AST, "foo").getLocation());
+ EXPECT_EQ(SM.getFileOffset(includeHashLoc(Foo, SM)),
+Test.llvm::Annotations::point("foo"));
+ FileID Bar = SM.getFileID(findDecl(AST, "bar").getLocation());
+ EXPECT_EQ(SM.getFileOffset(includeHashLoc(Bar, SM)),
+Test.llvm::Annotations::point("foo"));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
@@ -346,6 +346,25 @@
}
}
+TEST(SelectionTest, PathologicalPreprocessor) {
+ const char *Case = R"cpp(
+#define MACRO while(1)
+void test() {
+#include "Expand.inc"
+br^eak;
+}
+ )cpp";
+ Annotations Test(Case);
+ auto TU = TestTU::withCode(Test.code());
+ TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
+ auto AST = TU.build();
+ EXPECT_THAT(AST.getDiagnostics(), ::testing::IsEmpty());
+ auto T = makeSelectionTree(Case, AST);
+
+ EXPECT_EQ("BreakStmt", T.commonAncestor()->kind());
+ EXPECT_EQ("WhileStmt", T.commonAncestor()->Parent->kind());
+}
+
TEST(SelectionTest, Implicit) {
const char* Test = R"cpp(
struct S { S(const char*); };
Index: clang-tools-extra/trunk/clangd/SourceCode.cpp
[PATCH] D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include
sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clang-tools-extra/clangd/SourceCode.cpp:284 + return SM.getComposedLoc(IncludingFile, Offset); +if (Buf[Offset] == '\n' || Offset == 0) // no hash, what's going on? + return SourceLocation(); SureYeaah wrote: > nit: use this as a while condition? > > ``` > while(Offset-- && Buf[Offset] != '\n') > ``` Ooh, clever... a little too subtle for my taste though :-) (In particular, allowing the variable to overflow but then ensuring we never read from it...) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66590/new/ https://reviews.llvm.org/D66590 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include
SureYeaah accepted this revision. SureYeaah added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/SourceCode.cpp:284 + return SM.getComposedLoc(IncludingFile, Offset); +if (Buf[Offset] == '\n' || Offset == 0) // no hash, what's going on? + return SourceLocation(); nit: use this as a while condition? ``` while(Offset-- && Buf[Offset] != '\n') ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66590/new/ https://reviews.llvm.org/D66590 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include
sammccall marked 4 inline comments as done.
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/SourceCode.cpp:318
-// Check if two locations have the same file id.
-static bool inSameFile(SourceLocation Loc1, SourceLocation Loc2,
- const SourceManager ) {
I dropped this function because it's just SM.isSpelledInSameFile
Comment at: clang-tools-extra/clangd/SourceCode.cpp:338
+ : R2.getBegin();
+ SourceLocation End =
+ SM.isBeforeInTranslationUnit(getLocForTokenEnd(R1.getEnd(), SM,
LangOpts),
this is an unrelated bug fix: E1 < E2, std::min() etc don't do anything
sensible for SourceLocation
Comment at: clang-tools-extra/clangd/SourceCode.cpp:358
Begin = Begin.isFileID()
- ? SourceLocation()
+ ? includeHashLoc(SM.getFileID(Begin), SM)
: SM.getImmediateExpansionRange(Begin).getBegin()) {
This is half of the real fix: once we've hit the top of the macro tree, we
should walk up the `#include` stack in case we find a common ancestor there.
Comment at: clang-tools-extra/clangd/SourceCode.cpp:443
+ SourceRange Result =
+ rangeInCommonFile(unionTokenRange(R1, R2, SM, LangOpts), SM, LangOpts);
unsigned TokLen = getTokenLengthAtLoc(Result.getEnd(), SM, LangOpts);
this is one part of the main fix: the start and endpoint may be in different
real files in the presence of #include
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66590/new/
https://reviews.llvm.org/D66590
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include
sammccall updated this revision to Diff 216609.
sammccall added a comment.
kick phabricator to try to get it to mail
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66590/new/
https://reviews.llvm.org/D66590
Files:
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/unittests/SelectionTests.cpp
clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
Index: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
===
--- clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -11,9 +11,11 @@
#include "SourceCode.h"
#include "TestTU.h"
#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
#include "clang/Format/Format.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Testing/Support/Annotations.h"
#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -505,6 +507,53 @@
CheckRange("f");
}
+TEST(SourceCodeTests, HalfOpenFileRangePathologicalPreprocessor) {
+ const char *Case = R"cpp(
+#define MACRO while(1)
+void test() {
+[[#include "Expand.inc"
+br^eak]];
+}
+ )cpp";
+ Annotations Test(Case);
+ auto TU = TestTU::withCode(Test.code());
+ TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
+ auto AST = TU.build();
+
+ const auto = cast(findDecl(AST, "test"));
+ const auto = cast(Func.getBody());
+ const auto = cast(*Body->child_begin());
+ llvm::Optional Range = toHalfOpenFileRange(
+ AST.getSourceManager(), AST.getASTContext().getLangOpts(),
+ Loop->getSourceRange());
+ ASSERT_TRUE(Range) << "Failed to get file range";
+ EXPECT_EQ(AST.getSourceManager().getFileOffset(Range->getBegin()),
+Test.llvm::Annotations::range().Begin);
+ EXPECT_EQ(AST.getSourceManager().getFileOffset(Range->getEnd()),
+Test.llvm::Annotations::range().End);
+}
+
+TEST(SourceCodeTests, IncludeHashLoc) {
+ const char *Case = R"cpp(
+$foo^#include "foo.inc"
+#define HEADER "bar.inc"
+ $bar^# include HEADER
+ )cpp";
+ Annotations Test(Case);
+ auto TU = TestTU::withCode(Test.code());
+ TU.AdditionalFiles["foo.inc"] = "int foo;\n";
+ TU.AdditionalFiles["bar.inc"] = "int bar;\n";
+ auto AST = TU.build();
+ const auto& SM = AST.getSourceManager();
+
+ FileID Foo = SM.getFileID(findDecl(AST, "foo").getLocation());
+ EXPECT_EQ(SM.getFileOffset(includeHashLoc(Foo, SM)),
+Test.llvm::Annotations::point("foo"));
+ FileID Bar = SM.getFileID(findDecl(AST, "bar").getLocation());
+ EXPECT_EQ(SM.getFileOffset(includeHashLoc(Bar, SM)),
+Test.llvm::Annotations::point("foo"));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -346,6 +346,25 @@
}
}
+TEST(SelectionTest, PathologicalPreprocessor) {
+ const char *Case = R"cpp(
+#define MACRO while(1)
+void test() {
+#include "Expand.inc"
+br^eak;
+}
+ )cpp";
+ Annotations Test(Case);
+ auto TU = TestTU::withCode(Test.code());
+ TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
+ auto AST = TU.build();
+ EXPECT_THAT(AST.getDiagnostics(), ::testing::IsEmpty());
+ auto T = makeSelectionTree(Case, AST);
+
+ EXPECT_EQ("BreakStmt", T.commonAncestor()->kind());
+ EXPECT_EQ("WhileStmt", T.commonAncestor()->Parent->kind());
+}
+
TEST(SelectionTest, Implicit) {
const char* Test = R"cpp(
struct S { S(const char*); };
Index: clang-tools-extra/clangd/SourceCode.h
===
--- clang-tools-extra/clangd/SourceCode.h
+++ clang-tools-extra/clangd/SourceCode.h
@@ -83,6 +83,11 @@
/// the main file.
bool isInsideMainFile(SourceLocation Loc, const SourceManager );
+/// Returns the #include location through which IncludedFIle was loaded.
+/// Where SM.getIncludeLoc() returns the location of the *filename*, which may
+/// be in a macro, includeHashLoc() returns the location of the #.
+SourceLocation includeHashLoc(FileID IncludedFile, const SourceManager );
+
/// Returns true if the token at Loc is spelled in the source code.
/// This is not the case for:
/// * symbols formed via macro concatenation, the spelling location will
Index: clang-tools-extra/clangd/SourceCode.cpp
===
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -264,6 +264,29 @@
return L == R.getEnd() || halfOpenRangeContains(Mgr, R, L);
}
+SourceLocation includeHashLoc(FileID IncludedFile, const SourceManager ) {
+
[PATCH] D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include
sammccall created this revision.
sammccall added a reviewer: SureYeaah.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
https://github.com/clangd/clangd/issues/129
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D66590
Files:
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/unittests/SelectionTests.cpp
clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
Index: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
===
--- clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -11,9 +11,11 @@
#include "SourceCode.h"
#include "TestTU.h"
#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
#include "clang/Format/Format.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Testing/Support/Annotations.h"
#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -505,6 +507,53 @@
CheckRange("f");
}
+TEST(SourceCodeTests, HalfOpenFileRangePathologicalPreprocessor) {
+ const char *Case = R"cpp(
+#define MACRO while(1)
+void test() {
+[[#include "Expand.inc"
+br^eak]];
+}
+ )cpp";
+ Annotations Test(Case);
+ auto TU = TestTU::withCode(Test.code());
+ TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
+ auto AST = TU.build();
+
+ const auto = cast(findDecl(AST, "test"));
+ const auto = cast(Func.getBody());
+ const auto = cast(*Body->child_begin());
+ llvm::Optional Range = toHalfOpenFileRange(
+ AST.getSourceManager(), AST.getASTContext().getLangOpts(),
+ Loop->getSourceRange());
+ ASSERT_TRUE(Range) << "Failed to get file range";
+ EXPECT_EQ(AST.getSourceManager().getFileOffset(Range->getBegin()),
+Test.llvm::Annotations::range().Begin);
+ EXPECT_EQ(AST.getSourceManager().getFileOffset(Range->getEnd()),
+Test.llvm::Annotations::range().End);
+}
+
+TEST(SourceCodeTests, IncludeHashLoc) {
+ const char *Case = R"cpp(
+$foo^#include "foo.inc"
+#define HEADER "bar.inc"
+ $bar^# include HEADER
+ )cpp";
+ Annotations Test(Case);
+ auto TU = TestTU::withCode(Test.code());
+ TU.AdditionalFiles["foo.inc"] = "int foo;\n";
+ TU.AdditionalFiles["bar.inc"] = "int bar;\n";
+ auto AST = TU.build();
+ const auto& SM = AST.getSourceManager();
+
+ FileID Foo = SM.getFileID(findDecl(AST, "foo").getLocation());
+ EXPECT_EQ(SM.getFileOffset(includeHashLoc(Foo, SM)),
+Test.llvm::Annotations::point("foo"));
+ FileID Bar = SM.getFileID(findDecl(AST, "bar").getLocation());
+ EXPECT_EQ(SM.getFileOffset(includeHashLoc(Bar, SM)),
+Test.llvm::Annotations::point("foo"));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -346,6 +346,25 @@
}
}
+TEST(SelectionTest, PathologicalPreprocessor) {
+ const char *Case = R"cpp(
+#define MACRO while(1)
+void test() {
+#include "Expand.inc"
+br^eak;
+}
+ )cpp";
+ Annotations Test(Case);
+ auto TU = TestTU::withCode(Test.code());
+ TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
+ auto AST = TU.build();
+ EXPECT_THAT(AST.getDiagnostics(), ::testing::IsEmpty());
+ auto T = makeSelectionTree(Case, AST);
+
+ EXPECT_EQ("BreakStmt", T.commonAncestor()->kind());
+ EXPECT_EQ("WhileStmt", T.commonAncestor()->Parent->kind());
+}
+
TEST(SelectionTest, Implicit) {
const char* Test = R"cpp(
struct S { S(const char*); };
Index: clang-tools-extra/clangd/SourceCode.h
===
--- clang-tools-extra/clangd/SourceCode.h
+++ clang-tools-extra/clangd/SourceCode.h
@@ -83,6 +83,11 @@
/// the main file.
bool isInsideMainFile(SourceLocation Loc, const SourceManager );
+/// Returns the #include location through which IncludedFIle was loaded.
+/// Where SM.getIncludeLoc() returns the location of the *filename*, which may
+/// be in a macro, includeHashLoc() returns the location of the #.
+SourceLocation includeHashLoc(FileID IncludedFile, const SourceManager );
+
/// Returns true if the token at Loc is spelled in the source code.
/// This is not the case for:
/// * symbols formed via macro concatenation, the spelling location will
Index: clang-tools-extra/clangd/SourceCode.cpp
===
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -264,6 +264,29 @@
return L == R.getEnd() || halfOpenRangeContains(Mgr, R, L);
}
+SourceLocation includeHashLoc(FileID IncludedFile, const
