[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno marked 3 inline comments as done.
bruno added a comment.



> Do we need to perfectly preserve the functionality of `#pragma once` / `#import`? That is, would it be acceptable to 
>  you if we spuriously enter files that have been imported in that way, while building a module?

Unfortunately we can find ObjC headers that don't have include guards and only rely on #import's. Relaxing this condition could lead to more redefinition errors with current code base than we desire.

> Another possibility -- albeit slightly hacky -- would be to invent a controlling macro if the header in question turns out to not have one (say, form an identifier from the path by which the file was included and use that as the macro name) and is #imported / uses #pragma once. Or we could try simply rejecting textual headers that are #import'd / #pragma once'd and have no controlling macro in a modules build.

Right, it's not that hacky - it actually performs the job of actually guarding as we do with includes, but with more context. However, it's not clear to me how to determine the suggested 'identifier from the path'.

I have a third suggestion: seems like we only care here about two cases (1) modular import's from the same header - this will only happens with builtins AFAIU and (2) textual headers that must be imported multiple times for different modules. We can solve (1) by only using the information on HFI and fix (2) by only re-trying to enter the #imports where we find a controlling macro for the header and that macro isn't reachable at that point. I wrote a new patch that implements this, let me know what are your thoughts. Note that I intentionally left non-modular includes out because AFAIK they should be part of the first module they are founded, so it doesn't make any sense to re-enter them anyway.



================
Comment at: include/clang/Lex/HeaderSearch.h:98-101
+  /// List of modules that import from this header. Since the number of modules
+  /// using the same FileEntry is usually small (2 or 3 at most, often related
+  /// to builtins) this should be enough. Choose a more appropriate data
+  /// structure in case the requirement changes.
----------------
rsmith wrote:
> I don't think this is true in general. For a textual header, there could be hundreds or thousands of loaded modules that use it. If all header files in a project start with
> 
>   #include "local_config.h"
> 
> ... which is configured to be a textual header for whatever reason and contains a `#pragma once`, we would get into that situation for at least that one header. While a vector might be OK (as the number of entries should at least grow only linearly with the size of the entire codebase), quadratic-time algorithms over it probably won't be. Perhaps a sorted vector instead?
Right, I wasn't considering adding the textual headers at this point, this should have been filtered out by the ASTReader HFI merging. But handling the textual headers is necessary indeed. Hopefully my next patch fix it.


================
Comment at: lib/Lex/HeaderSearch.cpp:1116-1119
+    //  - if there not associated module or the module already imports
+    //    from this header.
+    //  - if any module associated with this header is visible.
+    if (FileInfo.isImport) {
----------------
rsmith wrote:
> I think this would be clearer as:
> 
> > [...], ignore it, unless doing so will lead to us importing a module that contains the file but didn't actually include it (because we're still building the corresponding module), and we've not already made the file visible by importing some other module.
> 
> ... but I don't think that's actually right. If `M` is null (if the file is only ever included as a textual header), we still need to check whether some visible module has actually made it visible, and we should enter it if not.
Looks like what I actually wanted was to check if modules were supported at this point with `LangOpts.Modules`


================
Comment at: lib/Serialization/ASTReader.cpp:1691-1692
     HFI.isModuleHeader |= !(HeaderRole & ModuleMap::TextualHeader);
+    if (HFI.isImport)
+      HFI.addModulesUsingHdr(Mod);
   }
----------------
rsmith wrote:
> Doing this here will do the wrong thing while building a module with local submodule visibility enabled -- we need to know whether the visible module set contains a module that entered the header, even for modules that we've never serialized.
> 
> Also, this will not populate the vector for the cases where the header was not listed in the module map, but was simply a non-moduler header that was textually entered while building a module.
Ok


https://reviews.llvm.org/D26267