sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous,
ilya-biryukov.
Herald added a project: clang.
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 <https://reviews.llvm.org/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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D78366
Files:
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
clang/lib/Lex/PPDirectives.cpp
Index: clang/lib/Lex/PPDirectives.cpp
===================================================================
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -1940,19 +1940,6 @@
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 @@
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 different callback for a pp-import?
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -33,6 +33,7 @@
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 @@
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
