On Thu, Aug 10, 2017 at 5:36 PM, Richard Smith <rich...@metafoo.co.uk> wrote: > On 10 August 2017 at 10:42, Hans Wennborg via cfe-commits > <cfe-commits@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.card...@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@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits