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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

Other options:

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? If we view them as pure optimization, we can do something //substantially// simpler here, for instance by never importing "isImported" flags from modules and throwing away all our "isImported" flags whenever module visibility decreases.

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.



================
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.
----------------
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?


================
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) {
----------------
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.


================
Comment at: lib/Serialization/ASTReader.cpp:1691-1692
     HFI.isModuleHeader |= !(HeaderRole & ModuleMap::TextualHeader);
+    if (HFI.isImport)
+      HFI.addModulesUsingHdr(Mod);
   }
----------------
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.


https://reviews.llvm.org/D26267