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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

LGTM. Thanks.



================
Comment at: clang/include/clang/Sema/Sema.h:2278
+
+  /// For an interface unit, this is the implicitly imported interface unit.
+  clang::Module *ThePrimaryInterface = nullptr;
----------------



================
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;
----------------
iains wrote:
> 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.
> 
Yeah, I tried to use hash algorithm to avoid the string comparison. But there is a concern that the hash algorithm is not safe. And I found the string comparison is not the bottle neck then. So I think it may not be too late to perform it actually if there is related bug report.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126959