r310605 - [Modules] Prevent #import to reenter header if not building a module.

[Bruno Cardoso Lopes via cfe-commits]

On Thu, Aug 10, 2017 at 5:36 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On 10 August 2017 at 10:42, Hans Wennborg via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
>>
>> Sounds good to me.
>>
>> Richard, what do you think?
>>
>> On Thu, Aug 10, 2017 at 9:38 AM, Bruno Cardoso Lopes
>> <bruno.cardoso at gmail.com> wrote:
>> > Hi Hans, can we please get this merged into 5.0?
>> >
>> > Thanks,
>> >
>> > On Thu, Aug 10, 2017 at 12:16 PM, Bruno Cardoso Lopes via cfe-commits
>> > <cfe-commits at lists.llvm.org> wrote:
>> >> Author: bruno
>> >> Date: Thu Aug 10 08:16:24 2017
>> >> New Revision: 310605
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=310605&view=rev
>> >> Log:
>> >> [Modules] Prevent #import to reenter header if not building a module.
>> >>
>> >> When non-modular headers are imported while not building a module but
>> >> in -fmodules mode, be conservative and preserve the default #import
>> >> semantic: do not reenter headers.
>
>
> This comment doesn't appear to describe what this patch does: even when
> building a module it does not re-enter headers except for ones declared
> 'textual header' within the module being built. (isCompilingModuleHeader
> doesn't mean "are we compiling a module header right now?", it means "is
> this file declared as some kind of header within the current
> CompilingModule?".)

What I'm trying to achieve here is: do not try to enter headers that
are not described as part of any module (textual) unless while
building a module. AFAIU, FileInfo.isCompilingModuleHeader is
basically LangOpts.isCompilingModule() && Mod->getTopLevelModule() ==
SourceModule, shouldn't it account for a more restrictive way to say
"are we compiling a textual header while building a module"?

Checking for FileInfo.isCompilingModuleHeader has the additional
advantage that it would track previous attempts to import that header
while building a module, in which case it's going to try to re-enter.
It won't try to re-enter only in cases where that header was never
imported while building a header -> the same behavior #import has
without modules.

Prior to r291644, clang would always avoid to re-enter a header. After
r291644 clang has a relaxed version of that, which I now proposing to
be less relaxed for #imports.

> I'm nervous about taking this change: it will presumably break some cases
> (particularly with local submodule visibility enabled) where a #pragma once
> header is #included into multiple headers in the same module, by refusing to
> re-enter such headers -- and it will fix other such cases, depending on what
> exactly the header does and the structure of the module.

I'd be interested in a such testcase to make sure we don't regress here!

> If this were restricted to the case where we're not building a module
> (!LangOpts.isCompilingModule()), then I'd be OK putting it on the branch.

Ok. It might not be a good idea to have this in 5.0 anyway, better
bake this for a while. But thinking forward, am I missing something
here? What about:

if ((LangOpts.isCompilingModule() || !isImport) && !FileInfo.isModuleHeader &&
    FileInfo.getControllingMacro(ExternalLookup))
  TryEnterHdr = true;

Thanks,

-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc