[PATCH] D123837: [C++20] [Modules] Judge isInCurrentModule currently

[Iain Sandoe via Phabricator via cfe-commits]

iains added inline comments.


================
Comment at: clang/include/clang/Basic/Module.h:537-543
+  static StringRef getPrimaryModuleInterfaceName(StringRef Name) {
+    return Name.split(':').first;
+  }
+
   /// Get the primary module interface name from a partition.
   StringRef getPrimaryModuleInterfaceName() const {
+    return getPrimaryModuleInterfaceName(getTopLevelModuleName());
----------------
I do not understand the purpose of this change, we already iterated over making the function more efficient.

For C++20 modules there cannot be more than one parent - so that the nested call to getTopLevelModule(), via getTopLevelModuleName() is redundant.

Is there some other case that needs special handling?
(If so then I think we should guard this at a higher level)



================
Comment at: clang/lib/Sema/SemaLookup.cpp:1567-1568
+
+  return M->getPrimaryModuleInterfaceName() ==
+         Module::getPrimaryModuleInterfaceName(LangOpts.CurrentModule);
 }
----------------
... of course, "correctness before optimisation", but this test function seems like it would benefit from some refactoring as suggested below.

it seems a bit unfortunate that we are placing a string comparison in the "acceptable decl" lookup path, which might be made many times (the previous use of the string compare was in making the module decl - which only happens once in a TU).

* Sema has visibility of the import graph of modules (via DirectModuleImports and Exports).
* We also know what the module parse state is (FirstDecl, GMF etc)
* If were are not parsing a module (i.e. LangOpts.CurrentModule.empty()) the we could return false early?
* if ModuleScopes is empty we know we are not (yet) parsing a module
* Once we are parsing a module then we can test ModuleScopes.back() to find the current module 

there are only two users of isInCurrentModule() - so maybe we refactor could make more use of the higher level state information - and / or cache information on the parents of partitions  to  avoid the complexity of parsing strings each time.

what do you think?


================
Comment at: clang/test/Modules/cxx20-10-1-ex2.cpp:62-69
+// According to [module.import]p7:
+//   Additionally, when a module-import-declaration in a module unit of some
+//   module M imports another module unit U of M, it also imports all 
+//   translation units imported by non-exported module-import-declarations in
+//   the module unit purview of U.
+//
+// So B is imported in B:X3 due to B:X2 imported B. So n is visible here.
----------------
@rsmith 

so, I wonder if this is intended, it seems to defeat the mechanisms to avoid circular dependencies.

TU1:
module M : Impl; // this is said not to generate a circular dependency because M : Impl does not implicitly import M

import M;  // but now we've done it manually ... and if that module is implicitly exported to importers of the Impl. module interface....
....

TU2:

export module M;
import : Impl; 

// ...  then boom! we have a circular dep.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123837/new/

https://reviews.llvm.org/D123837