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