On Tue, Nov 17, 2015 at 4:15 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
> 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. > Ah, I understand now - sorry for the confusion. 'spose at best we could 'ifdef CXX' the inclusions out, but that's more invasive, etc... :/ > > >> 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
