[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