Author: Sam McCall Date: 2020-04-20T17:28:42+02:00 New Revision: ee12edcb76423c78b55cdddae2edfe45cbb2ccd6
URL: https://github.com/llvm/llvm-project/commit/ee12edcb76423c78b55cdddae2edfe45cbb2ccd6 DIFF: https://github.com/llvm/llvm-project/commit/ee12edcb76423c78b55cdddae2edfe45cbb2ccd6.diff LOG: [Preamble] Allow recursive inclusion of header-guarded mainfile. Summary: This is guaranteed to be a no-op without the preamble, so should be a no-op with it too. Partially fixes https://github.com/clangd/clangd/issues/337 This doesn't yet work for #ifndef guards, which are not recognized in preambles. see D78038 I can't for the life of me work out how to test this outside clangd. The original reentrant preamble diagnostic was untested, I added a test to clangd for that too. Reviewers: kadircet Subscribers: ilya-biryukov, jkorous, arphaman, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D78366 Added: Modified: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp clang/lib/Lex/PPDirectives.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp index 00dd1f864813..f64f8e9901a9 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -33,6 +33,7 @@ using ::testing::ElementsAre; using ::testing::Field; using ::testing::IsEmpty; using ::testing::Pair; +using testing::SizeIs; using ::testing::UnorderedElementsAre; ::testing::Matcher<const Diag &> WithFix(::testing::Matcher<Fix> FixMatcher) { @@ -378,6 +379,47 @@ TEST(DiagnosticsTest, Preprocessor) { ElementsAre(Diag(Test.range(), "use of undeclared identifier 'b'"))); } +// Recursive main-file include is diagnosed, and doesn't crash. +TEST(DiagnosticsTest, RecursivePreamble) { + auto TU = TestTU::withCode(R"cpp( + #include "foo.h" // error-ok + int symbol; + )cpp"); + TU.Filename = "foo.h"; + EXPECT_THAT(TU.build().getDiagnostics(), + ElementsAre(DiagName("pp_including_mainfile_in_preamble"))); + EXPECT_THAT(TU.build().getLocalTopLevelDecls(), SizeIs(1)); +} + +// Recursive main-file include with #pragma once guard is OK. +TEST(DiagnosticsTest, RecursivePreamblePragmaOnce) { + auto TU = TestTU::withCode(R"cpp( + #pragma once + #include "foo.h" + int symbol; + )cpp"); + TU.Filename = "foo.h"; + EXPECT_THAT(TU.build().getDiagnostics(), IsEmpty()); + EXPECT_THAT(TU.build().getLocalTopLevelDecls(), SizeIs(1)); +} + +// Recursive main-file include with #ifndef guard should be OK. +// However, it's not yet recognized (incomplete at end of preamble). +TEST(DiagnosticsTest, RecursivePreambleIfndefGuard) { + auto TU = TestTU::withCode(R"cpp( + #ifndef FOO + #define FOO + #include "foo.h" // error-ok + int symbol; + #endif + )cpp"); + TU.Filename = "foo.h"; + // FIXME: should be no errors here. + EXPECT_THAT(TU.build().getDiagnostics(), + ElementsAre(DiagName("pp_including_mainfile_in_preamble"))); + EXPECT_THAT(TU.build().getLocalTopLevelDecls(), SizeIs(1)); +} + TEST(DiagnosticsTest, InsideMacros) { Annotations Test(R"cpp( #define TEN 10 diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp index 660c4a5e089d..d6b6f5695b6c 100644 --- a/clang/lib/Lex/PPDirectives.cpp +++ b/clang/lib/Lex/PPDirectives.cpp @@ -1940,19 +1940,6 @@ Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport( return {ImportAction::None}; } - // Check for circular inclusion of the main file. - // We can't generate a consistent preamble with regard to the conditional - // stack if the main file is included again as due to the preamble bounds - // some directives (e.g. #endif of a header guard) will never be seen. - // Since this will lead to confusing errors, avoid the inclusion. - if (File && PreambleConditionalStack.isRecording() && - SourceMgr.translateFile(&File->getFileEntry()) == - SourceMgr.getMainFileID()) { - Diag(FilenameTok.getLocation(), - diag::err_pp_including_mainfile_in_preamble); - return {ImportAction::None}; - } - // Should we enter the source file? Set to Skip if either the source file is // known to have no effect beyond its effect on module visibility -- that is, // if it's got an include guard that is already defined, set to Import if it @@ -2070,6 +2057,19 @@ Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport( Action = (SuggestedModule && !getLangOpts().CompilingPCH) ? Import : Skip; } + // Check for circular inclusion of the main file. + // We can't generate a consistent preamble with regard to the conditional + // stack if the main file is included again as due to the preamble bounds + // some directives (e.g. #endif of a header guard) will never be seen. + // Since this will lead to confusing errors, avoid the inclusion. + if (Action == Enter && File && PreambleConditionalStack.isRecording() && + SourceMgr.translateFile(&File->getFileEntry()) == + SourceMgr.getMainFileID()) { + Diag(FilenameTok.getLocation(), + diag::err_pp_including_mainfile_in_preamble); + return {ImportAction::None}; + } + if (Callbacks && !IsImportDecl) { // Notify the callback object that we've seen an inclusion directive. // FIXME: Use a diff erent callback for a pp-import? _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits