r253398 - [modules] When a #include is mapped to a module import and appears somewhere
David Blaikie via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 17 16:18:38 PST 2015
On Tue, Nov 17, 2015 at 4:15 PM, Richard Smith <richard at metafoo.co.uk>
wrote:
> On Tue, Nov 17, 2015 at 3:40 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Tue, Nov 17, 2015 at 3:32 PM, Richard Smith via cfe-commits <
>> cfe-commits at 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 at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151117/1d4ee2f1/attachment-0001.html>
More information about the cfe-commits
mailing list