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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
