Author: Kirill Bobyrev Date: 2022-04-21T16:54:59+02:00 New Revision: e1c0d2fb8272dd7f8e406334ac14077154217031
URL: https://github.com/llvm/llvm-project/commit/e1c0d2fb8272dd7f8e406334ac14077154217031 DIFF: https://github.com/llvm/llvm-project/commit/e1c0d2fb8272dd7f8e406334ac14077154217031.diff LOG: [clangd] Correctly identify self-contained headers included rercursively Right now when exiting the file Headers.cpp will identify the recursive inclusion (with a new FileID) as non self-contained and will add it to the set from which it will never be removed. As a result, we get incorrect results in the IncludeStructure and Include Cleaner. This patch is a fix. Reviewed By: sammccall Differential Revision: https://reviews.llvm.org/D124166 Added: Modified: clang-tools-extra/clangd/Headers.cpp clang-tools-extra/clangd/unittests/HeadersTests.cpp clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp index acc6421798755..4ee58f3b3ab52 100644 --- a/clang-tools-extra/clangd/Headers.cpp +++ b/clang-tools-extra/clangd/Headers.cpp @@ -106,10 +106,17 @@ class IncludeStructure::RecordHeaders : public PPCallbacks, InBuiltinFile = false; // At file exit time HeaderSearchInfo is valid and can be used to // determine whether the file was a self-contained header or not. - if (const FileEntry *FE = SM.getFileEntryForID(PrevFID)) - if (!isSelfContainedHeader(FE, PrevFID, SM, HeaderInfo)) + if (const FileEntry *FE = SM.getFileEntryForID(PrevFID)) { + // isSelfContainedHeader only returns true once the full header-guard + // structure has been seen, i.e. when exiting the *outer* copy of the + // file. So last result wins. + if (isSelfContainedHeader(FE, PrevFID, SM, HeaderInfo)) + Out->NonSelfContained.erase( + *Out->getID(SM.getFileEntryForID(PrevFID))); + else Out->NonSelfContained.insert( *Out->getID(SM.getFileEntryForID(PrevFID))); + } break; } case PPCallbacks::RenameFile: diff --git a/clang-tools-extra/clangd/unittests/HeadersTests.cpp b/clang-tools-extra/clangd/unittests/HeadersTests.cpp index c0cc3b69f3519..30bac23bc602c 100644 --- a/clang-tools-extra/clangd/unittests/HeadersTests.cpp +++ b/clang-tools-extra/clangd/unittests/HeadersTests.cpp @@ -383,6 +383,7 @@ TEST_F(HeadersTest, SelfContainedHeaders) { #include "nonguarded.h" #include "pp_depend.h" #include "pragmaguarded.h" +#include "recursive.h" )cpp"; FS.Files["pragmaguarded.h"] = R"cpp( #pragma once @@ -400,10 +401,19 @@ void foo(); # error You have to have PP directive set to include this one! #endif )cpp"; + FS.Files["recursive.h"] = R"cpp( + #ifndef RECURSIVE_H + #define RECURSIVE_H + + #include "recursive.h" + + #endif // RECURSIVE_H +)cpp"; auto Includes = collectIncludes(); EXPECT_TRUE(Includes.isSelfContained(getID("pragmaguarded.h", Includes))); EXPECT_TRUE(Includes.isSelfContained(getID("includeguarded.h", Includes))); + EXPECT_TRUE(Includes.isSelfContained(getID("recursive.h", Includes))); EXPECT_FALSE(Includes.isSelfContained(getID("nonguarded.h", Includes))); EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes))); } diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp index 4a376a87297f9..f28ffc5139f80 100644 --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -538,6 +538,38 @@ TEST(IncludeCleaner, IWYUPragmas) { EXPECT_THAT(Unused, IsEmpty()); } +TEST(IncludeCleaner, RecursiveInclusion) { + TestTU TU; + TU.Code = R"cpp( + #include "foo.h" + + void baz() { + foo(); + } + )cpp"; + TU.AdditionalFiles["foo.h"] = R"cpp( + #ifndef FOO_H + #define FOO_H + + void foo() {} + + #include "bar.h" + + #endif + )cpp"; + TU.AdditionalFiles["bar.h"] = guard(R"cpp( + #include "foo.h" + )cpp"); + ParsedAST AST = TU.build(); + + auto ReferencedFiles = findReferencedFiles( + findReferencedLocations(AST), AST.getIncludeStructure(), + AST.getCanonicalIncludes(), AST.getSourceManager()); + EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); + auto Unused = computeUnusedIncludes(AST); + EXPECT_THAT(Unused, IsEmpty()); +} + } // namespace } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
