[PATCH] D129174: [C++20][Modules] Update ADL to handle basic.lookup.argdep p4 [P1815R2 part 1]

[Iain Sandoe via Phabricator via cfe-commits]

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