iains added a comment.
sorry got a little delayed in responding to this (sorting out some testing
problems)
================
Comment at: clang/lib/Sema/SemaLookup.cpp:3859
+ // C++20 [basic.lookup.argdep] p4.3 .. are exported
+ Module *FM = D->getOwningModule();
+ // .. are attached to a named module M, do not appear in the
----------------
ChuanqiXu wrote:
> nit: Although it should be true due to D is ExportDeclContext, it looks
> better to add an assertion at the first sight.
OK, Since exports must be in module purview and not in PMF (which we should
expect from existing checks)
let's then check we have a correct context for export - which will avoid
needing to re-check this below.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:3862-3863
+ // translation unit containing the point of the lookup..
+ if (FM->isModulePurview() &&
+ (ModuleScopes.empty() || FM != ModuleScopes.back().Module)) {
+ for (auto *E : AssociatedClasses) {
----------------
ChuanqiXu wrote:
>
OK, we can use this here, although it is probably a little more than necessary,
since we already know that FM cannot be a GMF or PMF ( but the function will
check those anyway)
================
Comment at: clang/lib/Sema/SemaLookup.cpp:3864-3878
+ for (auto *E : AssociatedClasses) {
+ // and have the same innermost enclosing non-inline namespace
+ // scope as a declaration of an associated entity attached to M
+ if (!E->hasOwningModule() || E->getOwningModule() != FM)
+ continue;
+ // TODO: maybe this could be cached when generating the
+ // associated namespaces / entities.
----------------
ChuanqiXu wrote:
> nit: how do you think about the suggested style? (not required)
I guess it's a little shorter, and roughly the same in readability,
================
Comment at: clang/lib/Sema/SemaLookup.cpp:3867
+ // scope as a declaration of an associated entity attached to M
+ if (!E->hasOwningModule() || E->getOwningModule() != FM)
+ continue;
----------------
ChuanqiXu wrote:
> The std says `module` instead of `module unit`. So I think we should consider
> partition unit here. So:
> - We need to add a test about partition in the revision.
> - We need to add `isInSameModule` methods in other revisions. I know we've
> talked this several times before... I'll take a look.
> The std says `module` instead of `module unit`. So I think we should consider
> partition unit here.
yes, I guess so - it is a bit unfortunate that we need to use the name
comparison again.
> So:
> - We need to add a test about partition in the revision.
TODO, probably better to add another example rather than modify the text of the
standard one.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:3871-3873
+ DeclContext *Ctx = E->getDeclContext();
+ while (!Ctx->isFileContext() || Ctx->isInlineNamespace())
+ Ctx = Ctx->getParent();
----------------
ChuanqiXu wrote:
> Maybe it is worth to implement `getNonInlineEnclosingNamespaceContext` and we
> could simplify the code here:
Perhaps. but I would prefer that to be a separate cleanup - since it would be
touching code unrelated to this.
Also that does not actually save any work, it might be better to find a way to
cache the computation at the time the associated entities are found.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129174/new/
https://reviews.llvm.org/D129174
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
