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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
