rsmith added inline comments.

================
Comment at: lib/Sema/SemaDecl.cpp:16135
+    for (Module *ImportedModule : Mod->Imports)
+      VisibleModules.setVisible(ImportedModule, ModuleLoc);
+  }
----------------
For completeness, you should also call 
`getModuleLoader().makeModuleVisible(Mod, Module::AllVisible, Loc);` here. (It 
turns out to not actually matter right now because `ModulesTS` implies 
`LocalSubmoduleVisibility`, under which `makeModuleVisible` happens to be a 
no-op.)


================
Comment at: lib/Sema/SemaDecl.cpp:16185-16194
+  if (getLangOpts().ModulesTS) {
+    Module *CurrentModule = getCurrentModule();
+    assert(CurrentModule && "Expected to be in a module scope");
+
+    // If the current module has been loaded from disk, then this is an
+    // implementation unit and hence we shouldn't modify the module.
+    // FIXME: Is that a hacky assumption? We can't just check
----------------
This is not appropriate; generally whether we serialize to an AST file should 
be treated as orthogonal to whether we're in / importing a module.

The right check here is probably `getLangOpts().getCompilingModule() == 
CMK_ModuleInterface`.


https://reviews.llvm.org/D40443



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to