Re: r291628 - Module: Do not create Implicit ImportDecl for module X if we

[Manman Ren via cfe-commits]

On Tue, Jan 10, 2017 at 5:59 PM, Richard Smith 
wrote:

> On 10 January 2017 at 17:57, Richard Smith  wrote:
>
>> On 10 January 2017 at 16:48, Manman Ren via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: mren
>>> Date: Tue Jan 10 18:48:19 2017
>>> New Revision: 291628
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=291628=rev
>>> Log:
>>> Module: Do not create Implicit ImportDecl for module X if we
>>>  are building an implemenation of module X.
>>>
>>
>> Hmm. We do actually have an include mapping to a module header in this
>> case. Perhaps it would be better to create a faithful AST representation
>> and handle this in CodeGen instead -- we should not add any link flags when
>> an implementation TU of a module imports a header of that same module.
>>
>
> It's worth pointing out that the C++ modules TS allows a modular import of
> a module interface into an implementation unit of that module, so the
> current approach won't work for that case, but filtering these imports out
> in CodeGen would work.
>

Makes sense. I will commit a follow-up patch.

Manman

>
>
>> This fixes a regression caused by r280409.
>>> rdar://problem/29930553
>>>
>>> Added:
>>> cfe/trunk/test/Modules/Inputs/module-impl-with-link/
>>> cfe/trunk/test/Modules/Inputs/module-impl-with-link/foo.h
>>> cfe/trunk/test/Modules/Inputs/module-impl-with-link/module.modulemap
>>> cfe/trunk/test/Modules/module-impl-with-link.c
>>> Modified:
>>> cfe/trunk/include/clang/Sema/Sema.h
>>> cfe/trunk/lib/Sema/SemaDecl.cpp
>>>
>>> Modified: cfe/trunk/include/clang/Sema/Sema.h
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/
>>> Sema/Sema.h?rev=291628=291627=291628=diff
>>> 
>>> ==
>>> --- cfe/trunk/include/clang/Sema/Sema.h (original)
>>> +++ cfe/trunk/include/clang/Sema/Sema.h Tue Jan 10 18:48:19 2017
>>> @@ -1905,7 +1905,8 @@ public:
>>>/// \brief The parser has processed a module import translated from a
>>>/// #include or similar preprocessing directive.
>>>void ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod);
>>> -  void BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod);
>>> +  void BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod,
>>> +  bool NoImport);
>>>
>>>/// \brief The parsed has entered a submodule.
>>>void ActOnModuleBegin(SourceLocation DirectiveLoc, Module *Mod);
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaD
>>> ecl.cpp?rev=291628=291627=291628=diff
>>> 
>>> ==
>>> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Tue Jan 10 18:48:19 2017
>>> @@ -15652,10 +15652,11 @@ DeclResult Sema::ActOnModuleImport(Sourc
>>>
>>>  void Sema::ActOnModuleInclude(SourceLocation DirectiveLoc, Module
>>> *Mod) {
>>>checkModuleImportContext(*this, Mod, DirectiveLoc, CurContext, true);
>>> -  BuildModuleInclude(DirectiveLoc, Mod);
>>> +  BuildModuleInclude(DirectiveLoc, Mod, false/*NoImport*/);
>>>  }
>>>
>>> -void Sema::BuildModuleInclude(SourceLocation DirectiveLoc, Module
>>> *Mod) {
>>> +void Sema::BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod,
>>> +  bool NoImport) {
>>>// 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
>>>// implementation detail of us building the module.
>>> @@ -15665,7 +15666,7 @@ void Sema::BuildModuleInclude(SourceLoca
>>>TUKind == TU_Module &&
>>>getSourceManager().isWrittenInMainFile(DirectiveLoc);
>>>
>>> -  bool ShouldAddImport = !IsInModuleIncludes;
>>> +  bool ShouldAddImport = !IsInModuleIncludes && !NoImport;
>>>
>>>// If this module import was due to an inclusion directive, create an
>>>// implicit import declaration to capture it in the AST.
>>> @@ -15713,7 +15714,11 @@ void Sema::ActOnModuleEnd(SourceLocation
>>>assert(File != getSourceManager().getMainFileID() &&
>>>   "end of submodule in main source file");
>>>SourceLocation DirectiveLoc = getSourceManager().getIncludeLoc(File);
>>> -  BuildModuleInclude(DirectiveLoc, Mod);
>>> +  // Do not create implicit ImportDecl if we are building the
>>> implementation
>>> +  // of a module.
>>> +  bool NoImport = Mod->getTopLevelModuleName() ==
>>> getLangOpts().CurrentModule &&
>>> +  !getLangOpts().isCompilingModule();
>>> +  BuildModuleInclude(DirectiveLoc, Mod, NoImport);
>>>  }
>>>
>>>  void Sema::createImplicitModuleImportForErrorRecovery(SourceLocation
>>> Loc,
>>>
>>> Added: cfe/trunk/test/Modules/Inputs/module-impl-with-link/foo.h
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/I
>>> 

Re: r291628 - Module: Do not create Implicit ImportDecl for module X if we

[Richard Smith via cfe-commits]

On 10 January 2017 at 17:57, Richard Smith  wrote:

> On 10 January 2017 at 16:48, Manman Ren via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: mren
>> Date: Tue Jan 10 18:48:19 2017
>> New Revision: 291628
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=291628=rev
>> Log:
>> Module: Do not create Implicit ImportDecl for module X if we
>>  are building an implemenation of module X.
>>
>
> Hmm. We do actually have an include mapping to a module header in this
> case. Perhaps it would be better to create a faithful AST representation
> and handle this in CodeGen instead -- we should not add any link flags when
> an implementation TU of a module imports a header of that same module.
>

It's worth pointing out that the C++ modules TS allows a modular import of
a module interface into an implementation unit of that module, so the
current approach won't work for that case, but filtering these imports out
in CodeGen would work.


> This fixes a regression caused by r280409.
>> rdar://problem/29930553
>>
>> Added:
>> cfe/trunk/test/Modules/Inputs/module-impl-with-link/
>> cfe/trunk/test/Modules/Inputs/module-impl-with-link/foo.h
>> cfe/trunk/test/Modules/Inputs/module-impl-with-link/module.modulemap
>> cfe/trunk/test/Modules/module-impl-with-link.c
>> Modified:
>> cfe/trunk/include/clang/Sema/Sema.h
>> cfe/trunk/lib/Sema/SemaDecl.cpp
>>
>> Modified: cfe/trunk/include/clang/Sema/Sema.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/
>> Sema/Sema.h?rev=291628=291627=291628=diff
>> 
>> ==
>> --- cfe/trunk/include/clang/Sema/Sema.h (original)
>> +++ cfe/trunk/include/clang/Sema/Sema.h Tue Jan 10 18:48:19 2017
>> @@ -1905,7 +1905,8 @@ public:
>>/// \brief The parser has processed a module import translated from a
>>/// #include or similar preprocessing directive.
>>void ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod);
>> -  void BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod);
>> +  void BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod,
>> +  bool NoImport);
>>
>>/// \brief The parsed has entered a submodule.
>>void ActOnModuleBegin(SourceLocation DirectiveLoc, Module *Mod);
>>
>> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaD
>> ecl.cpp?rev=291628=291627=291628=diff
>> 
>> ==
>> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Tue Jan 10 18:48:19 2017
>> @@ -15652,10 +15652,11 @@ DeclResult Sema::ActOnModuleImport(Sourc
>>
>>  void Sema::ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod)
>> {
>>checkModuleImportContext(*this, Mod, DirectiveLoc, CurContext, true);
>> -  BuildModuleInclude(DirectiveLoc, Mod);
>> +  BuildModuleInclude(DirectiveLoc, Mod, false/*NoImport*/);
>>  }
>>
>> -void Sema::BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod)
>> {
>> +void Sema::BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod,
>> +  bool NoImport) {
>>// 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
>>// implementation detail of us building the module.
>> @@ -15665,7 +15666,7 @@ void Sema::BuildModuleInclude(SourceLoca
>>TUKind == TU_Module &&
>>getSourceManager().isWrittenInMainFile(DirectiveLoc);
>>
>> -  bool ShouldAddImport = !IsInModuleIncludes;
>> +  bool ShouldAddImport = !IsInModuleIncludes && !NoImport;
>>
>>// If this module import was due to an inclusion directive, create an
>>// implicit import declaration to capture it in the AST.
>> @@ -15713,7 +15714,11 @@ void Sema::ActOnModuleEnd(SourceLocation
>>assert(File != getSourceManager().getMainFileID() &&
>>   "end of submodule in main source file");
>>SourceLocation DirectiveLoc = getSourceManager().getIncludeLoc(File);
>> -  BuildModuleInclude(DirectiveLoc, Mod);
>> +  // Do not create implicit ImportDecl if we are building the
>> implementation
>> +  // of a module.
>> +  bool NoImport = Mod->getTopLevelModuleName() ==
>> getLangOpts().CurrentModule &&
>> +  !getLangOpts().isCompilingModule();
>> +  BuildModuleInclude(DirectiveLoc, Mod, NoImport);
>>  }
>>
>>  void Sema::createImplicitModuleImportForErrorRecovery(SourceLocation
>> Loc,
>>
>> Added: cfe/trunk/test/Modules/Inputs/module-impl-with-link/foo.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/
>> Inputs/module-impl-with-link/foo.h?rev=291628=auto
>> 
>> ==
>> --- cfe/trunk/test/Modules/Inputs/module-impl-with-link/foo.h (added)
>> +++ 

r291628 - Module: Do not create Implicit ImportDecl for module X if we

[Manman Ren via cfe-commits]

Author: mren
Date: Tue Jan 10 18:48:19 2017
New Revision: 291628

URL: http://llvm.org/viewvc/llvm-project?rev=291628=rev
Log:
Module: Do not create Implicit ImportDecl for module X if we
 are building an implemenation of module X.

This fixes a regression caused by r280409.
rdar://problem/29930553

Added:
cfe/trunk/test/Modules/Inputs/module-impl-with-link/
cfe/trunk/test/Modules/Inputs/module-impl-with-link/foo.h
cfe/trunk/test/Modules/Inputs/module-impl-with-link/module.modulemap
cfe/trunk/test/Modules/module-impl-with-link.c
Modified:
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaDecl.cpp

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=291628=291627=291628=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Tue Jan 10 18:48:19 2017
@@ -1905,7 +1905,8 @@ public:
   /// \brief The parser has processed a module import translated from a
   /// #include or similar preprocessing directive.
   void ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod);
-  void BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod);
+  void BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod,
+  bool NoImport);
 
   /// \brief The parsed has entered a submodule.
   void ActOnModuleBegin(SourceLocation DirectiveLoc, Module *Mod);

Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=291628=291627=291628=diff
==
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Tue Jan 10 18:48:19 2017
@@ -15652,10 +15652,11 @@ DeclResult Sema::ActOnModuleImport(Sourc
 
 void Sema::ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod) {
   checkModuleImportContext(*this, Mod, DirectiveLoc, CurContext, true);
-  BuildModuleInclude(DirectiveLoc, Mod);
+  BuildModuleInclude(DirectiveLoc, Mod, false/*NoImport*/);
 }
 
-void Sema::BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod) {
+void Sema::BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod,
+  bool NoImport) {
   // 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
   // implementation detail of us building the module.
@@ -15665,7 +15666,7 @@ void Sema::BuildModuleInclude(SourceLoca
   TUKind == TU_Module &&
   getSourceManager().isWrittenInMainFile(DirectiveLoc);
 
-  bool ShouldAddImport = !IsInModuleIncludes;
+  bool ShouldAddImport = !IsInModuleIncludes && !NoImport;
 
   // If this module import was due to an inclusion directive, create an
   // implicit import declaration to capture it in the AST.
@@ -15713,7 +15714,11 @@ void Sema::ActOnModuleEnd(SourceLocation
   assert(File != getSourceManager().getMainFileID() &&
  "end of submodule in main source file");
   SourceLocation DirectiveLoc = getSourceManager().getIncludeLoc(File);
-  BuildModuleInclude(DirectiveLoc, Mod);
+  // Do not create implicit ImportDecl if we are building the implementation
+  // of a module.
+  bool NoImport = Mod->getTopLevelModuleName() == getLangOpts().CurrentModule 
&&
+  !getLangOpts().isCompilingModule();
+  BuildModuleInclude(DirectiveLoc, Mod, NoImport);
 }
 
 void Sema::createImplicitModuleImportForErrorRecovery(SourceLocation Loc,

Added: cfe/trunk/test/Modules/Inputs/module-impl-with-link/foo.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/module-impl-with-link/foo.h?rev=291628=auto
==
--- cfe/trunk/test/Modules/Inputs/module-impl-with-link/foo.h (added)
+++ cfe/trunk/test/Modules/Inputs/module-impl-with-link/foo.h Tue Jan 10 
18:48:19 2017
@@ -0,0 +1 @@
+//empty

Added: cfe/trunk/test/Modules/Inputs/module-impl-with-link/module.modulemap
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/module-impl-with-link/module.modulemap?rev=291628=auto
==
--- cfe/trunk/test/Modules/Inputs/module-impl-with-link/module.modulemap (added)
+++ cfe/trunk/test/Modules/Inputs/module-impl-with-link/module.modulemap Tue 
Jan 10 18:48:19 2017
@@ -0,0 +1,4 @@
+module Clib {
+  header "foo.h"
+  link "Clib"
+}

Added: cfe/trunk/test/Modules/module-impl-with-link.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/module-impl-with-link.c?rev=291628=auto
==
--- cfe/trunk/test/Modules/module-impl-with-link.c (added)
+++ cfe/trunk/test/Modules/module-impl-with-link.c Tue Jan 10 18:48:19 2017
@@ -0,0 +1,7 @@
+//