<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Nov 17, 2015 at 3:40 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Tue, Nov 17, 2015 at 3:32 PM, Richard Smith via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: rsmith<br>
Date: Tue Nov 17 17:32:01 2015<br>
New Revision: 253398<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=253398&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=253398&view=rev</a><br>
Log:<br>
[modules] When a #include is mapped to a module import and appears somewhere<br>
other than the top level, we issue an error. This breaks a fair amount of C++<br>
code wrapping C libraries, where the C library is #included within a namespace<br>
/ extern "C" combination, because the C library (probably) includes C++<br>
standard library headers which may be within modules.<br>
<br>
Without modules, this setup is harmless if (and *only* if) the corresponding<br>
standard library module was already included outside the namespace,</blockquote><div><br></div></span><div>^ if that's the only place it's valid, is it that much more work to just remove all such inclusions as pointless?</div></div></div></div></blockquote><div><br></div><div>Yes, and that would break the build of the third-party library in a lot of cases. The pattern looks like this:</div><div><br></div><div>c_header.h:</div><div>#include <stdio.h></div><div>my decls(); // use stuff from stdio.h here</div><div><br></div><div>cxx_header.h:</div><div>#include <stdio.h> // important, do not remove</div><div>namespace WrappedCLibrary {</div><div>extern "C" {</div><div>#include "c_header.h"</div><div>}</div><div>}</div><div><br></div><div>Note that removing the include from c_header.h will potentially break all the other (C) users of that header.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>If they ever trigger they'll do bad things, yes?<br><br>(but I guess on a large enough codebase this fight is not worth fighting in the effort to transition to modules? even if it's just deletions (code review turnaround, etc))</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> so<br>
downgrade the error to a default-error extension in that case, so that it can<br>
be selectively disabled for such misbehaving libraries.<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
cfe/trunk/lib/Parse/ParseDecl.cpp<br>
cfe/trunk/lib/Parse/ParseDeclCXX.cpp<br>
cfe/trunk/lib/Parse/ParseStmt.cpp<br>
cfe/trunk/lib/Sema/SemaDecl.cpp<br>
cfe/trunk/test/Modules/auto-module-import.m<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=253398&r1=253397&r2=253398&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=253398&r1=253397&r2=253398&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Nov 17 17:32:01 2015<br>
@@ -7930,6 +7930,9 @@ def note_module_import_in_extern_c : Not<br>
"extern \"C\" language linkage specification begins here">;<br>
def err_module_import_not_at_top_level_fatal : Error<<br>
"import of module '%0' appears within %1">, DefaultFatal;<br>
+def ext_module_import_not_at_top_level_noop : ExtWarn<<br>
+ "redundant #include of module '%0' appears within %1">, DefaultError,<br>
+ InGroup<DiagGroup<"modules-import-nested-redundant">>;<br>
def note_module_import_not_at_top_level : Note<"%0 begins here">;<br>
def err_module_self_import : Error<<br>
"import of module '%0' appears within same top-level module '%1'">;<br>
<br>
Modified: cfe/trunk/lib/Parse/ParseDecl.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDecl.cpp?rev=253398&r1=253397&r2=253398&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDecl.cpp?rev=253398&r1=253397&r2=253398&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Parse/ParseDecl.cpp (original)<br>
+++ cfe/trunk/lib/Parse/ParseDecl.cpp Tue Nov 17 17:32:01 2015<br>
@@ -3589,8 +3589,8 @@ void Parser::ParseStructUnionBody(Source<br>
SmallVector<Decl *, 32> FieldDecls;<br>
<br>
// While we still have something to read, read the declarations in the struct.<br>
- while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof) &&<br>
- !tryParseMisplacedModuleImport()) {<br>
+ while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&<br>
+ Tok.isNot(tok::eof)) {<br>
// Each iteration of this loop reads one struct-declaration.<br>
<br>
// Check for extraneous top-level semicolon.<br>
<br>
Modified: cfe/trunk/lib/Parse/ParseDeclCXX.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDeclCXX.cpp?rev=253398&r1=253397&r2=253398&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDeclCXX.cpp?rev=253398&r1=253397&r2=253398&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Parse/ParseDeclCXX.cpp (original)<br>
+++ cfe/trunk/lib/Parse/ParseDeclCXX.cpp Tue Nov 17 17:32:01 2015<br>
@@ -210,8 +210,8 @@ void Parser::ParseInnerNamespace(std::ve<br>
ParsedAttributes &attrs,<br>
BalancedDelimiterTracker &Tracker) {<br>
if (index == Ident.size()) {<br>
- while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof) &&<br>
- !tryParseMisplacedModuleImport()) {<br>
+ while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&<br>
+ Tok.isNot(tok::eof)) {<br>
ParsedAttributesWithRange attrs(AttrFactory);<br>
MaybeParseCXX11Attributes(attrs);<br>
MaybeParseMicrosoftAttributes(attrs);<br>
@@ -3064,8 +3064,8 @@ void Parser::ParseCXXMemberSpecification<br>
<br>
if (TagDecl) {<br>
// While we still have something to read, read the member-declarations.<br>
- while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof) &&<br>
- !tryParseMisplacedModuleImport()) {<br>
+ while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&<br>
+ Tok.isNot(tok::eof)) {<br>
// Each iteration of this loop reads one member-declaration.<br>
ParseCXXClassMemberDeclarationWithPragmas(<br>
CurAS, AccessAttrs, static_cast<DeclSpec::TST>(TagType), TagDecl);<br>
<br>
Modified: cfe/trunk/lib/Parse/ParseStmt.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseStmt.cpp?rev=253398&r1=253397&r2=253398&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseStmt.cpp?rev=253398&r1=253397&r2=253398&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Parse/ParseStmt.cpp (original)<br>
+++ cfe/trunk/lib/Parse/ParseStmt.cpp Tue Nov 17 17:32:01 2015<br>
@@ -948,8 +948,8 @@ StmtResult Parser::ParseCompoundStatemen<br>
Stmts.push_back(R.get());<br>
}<br>
<br>
- while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof) &&<br>
- !tryParseMisplacedModuleImport()) {<br>
+ while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&<br>
+ Tok.isNot(tok::eof)) {<br>
if (Tok.is(tok::annot_pragma_unused)) {<br>
HandlePragmaUnused();<br>
continue;<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaDecl.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=253398&r1=253397&r2=253398&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=253398&r1=253397&r2=253398&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Tue Nov 17 17:32:01 2015<br>
@@ -14500,8 +14500,8 @@ Decl *Sema::ActOnFileScopeAsmDecl(Expr *<br>
}<br>
<br>
static void checkModuleImportContext(Sema &S, Module *M,<br>
- SourceLocation ImportLoc,<br>
- DeclContext *DC) {<br>
+ SourceLocation ImportLoc, DeclContext *DC,<br>
+ bool FromInclude = false) {<br>
SourceLocation ExternCLoc;<br>
<br>
if (auto *LSD = dyn_cast<LinkageSpecDecl>(DC)) {<br>
@@ -14520,7 +14520,9 @@ static void checkModuleImportContext(Sem<br>
DC = DC->getParent();<br>
<br>
if (!isa<TranslationUnitDecl>(DC)) {<br>
- S.Diag(ImportLoc, diag::err_module_import_not_at_top_level_fatal)<br>
+ S.Diag(ImportLoc, (FromInclude && S.isModuleVisible(M))<br>
+ ? diag::ext_module_import_not_at_top_level_noop<br>
+ : diag::err_module_import_not_at_top_level_fatal)<br>
<< M->getFullModuleName() << DC;<br>
S.Diag(cast<Decl>(DC)->getLocStart(),<br>
diag::note_module_import_not_at_top_level) << DC;<br>
@@ -14579,7 +14581,7 @@ DeclResult Sema::ActOnModuleImport(Sourc<br>
}<br>
<br>
void Sema::ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod) {<br>
- checkModuleImportContext(*this, Mod, DirectiveLoc, CurContext);<br>
+ checkModuleImportContext(*this, Mod, DirectiveLoc, CurContext, true);<br>
<br>
// Determine whether we're in the #include buffer for a module. The #includes<br>
// in that buffer do not qualify as module imports; they're just an<br>
<br>
Modified: cfe/trunk/test/Modules/auto-module-import.m<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/auto-module-import.m?rev=253398&r1=253397&r2=253398&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/auto-module-import.m?rev=253398&r1=253397&r2=253398&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Modules/auto-module-import.m (original)<br>
+++ cfe/trunk/test/Modules/auto-module-import.m Tue Nov 17 17:32:01 2015<br>
@@ -1,6 +1,7 @@<br>
// RUN: rm -rf %t<br>
// RUN: %clang_cc1 -Wauto-import -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -F %S/Inputs %s -verify -DERRORS<br>
// RUN: %clang_cc1 -Wauto-import -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -F %S/Inputs %s -verify<br>
+// RUN: %clang_cc1 -Wauto-import -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -F %S/Inputs -xobjective-c++ %s -verify<br>
//<br>
// Test both with and without the declarations that refer to unimported<br>
// entities. For error recovery, those cases implicitly trigger an import.<br>
@@ -85,5 +86,16 @@ int getNotInModule() {<br>
<br>
void includeNotAtTopLevel() { // expected-note {{function 'includeNotAtTopLevel' begins here}}<br>
#include <NoUmbrella/A.h> // expected-warning {{treating #include as an import}} \<br>
- expected-error {{import of module 'NoUmbrella.A' appears within function 'includeNotAtTopLevel'}}<br>
+ expected-error {{redundant #include of module 'NoUmbrella.A' appears within function 'includeNotAtTopLevel'}}<br>
}<br>
+<br>
+#ifdef __cplusplus<br>
+namespace NS { // expected-note {{begins here}}<br>
+#include <NoUmbrella/A.h> // expected-warning {{treating #include as an import}} \<br>
+ expected-error {{redundant #include of module 'NoUmbrella.A' appears within namespace 'NS'}}<br>
+}<br>
+extern "C" { // expected-note {{begins here}}<br>
+#include <NoUmbrella/A.h> // expected-warning {{treating #include as an import}} \<br>
+ expected-error {{import of C++ module 'NoUmbrella.A' appears within extern "C"}}<br>
+}<br>
+#endif<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>