[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

[Iain Sandoe via Phabricator via cfe-commits]

iains marked an inline comment as done.
iains added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:1672-1677
+  // A module implementation unit has visibility of the decls in its implicitly
+  // imported interface.
+  if (getLangOpts().CPlusPlusModules && NewM && OldM &&
+      NewM->Kind == Module::ModuleImplementationUnit &&
+      OldM->Kind == Module::ModuleInterfaceUnit && NewM->Name == OldM->Name)
+    return false;
----------------
ChuanqiXu wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > I feel slightly better to add a field in Sema:
> > > 
> > > ```
> > > Module *PrimaryModuleInterface = nullptr; // Only valid if we're parsing a module unit.
> > > ```
> > > 
> > > Then we can avoid the string compare here.  Also we can add it to `Sema::isUsableModule`. Now `Sema::isUsableModule` works because it falls to the string comparison.
> > I also thought about doing this, but decided that we should try to keep the interface regular - so that all modules are treated the same.
> > 
> > If we find that the string comparison for primary module name is a "hot spot" then we should address it a different way - by caching an identifier or similar (since we need to compare it in other places too).
> It just smells bad. I did profiling for the performance for modules these days. And it looks like the string comparison can hard to be the hot spot. The bottleneck lives in other places. The string comparison may be in the long tail. So we might not be able to prove this one by giving numbers. But it indeed smells bad and we can solve it simply. So let's make it. I think it is important to improve our code quality.
I have done this (for now) to make progress.

but, actually, in this case, I do not agree with the mechanism

.. instead of making this into a special case we should (as part of the following fixes to lookup) make efficient helpers in module / sema that allow us to see "same TU", "same named module" etc.  That way we can improve the performance of any one of those if it becomes an issue.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126959