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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> The lookup problems might be considered to be quite a serious bug, and so we should consider possible back porting and try to avoid making large changes in theses patches (once that problem is solved, then we can refactor, I think).

I agree it is important to fix the bug. But I don't think we should back port this one to 16. On the one hand, this is not a regression bug. On the other hand, the existing users (we already have a lot users now) didn't report such problems. So I prefer to target 17 for this. And I don't mind to do refactoring after landing the fixing patch. Since we're looking at the code actively.



================
Comment at: clang/include/clang/Basic/Module.h:137
     ImplicitGlobalModuleFragment,
   };
 
----------------
iains wrote:
> ChuanqiXu wrote:
> > We may be able to save 1 bit for ModuleKind by adding two field bits to Module: `IsImplementation` and `IsPartition`. Then we can remove `ModulePartitionInterface` and `ModulePartitionImplementation`. Then let's rename `ModuleInterfaceUnit` to `NamedModuleUnit`.
> > So we can judge module interface unit, implementation unit,  module partition interface and module partition implementation by `NamedModuleUnit ` and the two bits.
> Yes I agree we could do this, but let's keep refactoring as a follow-on job.
OK. I don't think this is a blocking issue too. Let's have one FIXME for it.


================
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:
> > 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.


================
Comment at: clang/test/CXX/module/basic/basic.def.odr/p4.cppm:155-156
+
+  // FIXME: Issue #61427 Internal-linkage declarations in the interface TU
+  // should not be not visible here.
   (void)&static_var_module_linkage; // FIXME: Should not be visible here.
----------------
iains wrote:
> ChuanqiXu wrote:
> > nit: I am a little bit tired to add FIXME in the tests... let's not try to add more...
> I agree that FIXMEs in tests are likely to be forgotten, so that they are not so much use.
> 
> However, we have a situation in which the test needs to be incorrect in order to pass at the present time; we do not have a good mechanism for noting this - since if we add the expected-error, then the test will fail - so we'd have to XFAIL it - which would mean that we would lose test coverage for the cases that do work.
> 
> If you prefer, I can remove the FIXME.
I don't feel bad to remain this one. Since it is a little bit special as you said.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126959