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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
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
----------------
nit: Although it should be true due to D is ExportDeclContext, it looks better to add an assertion at the first sight.


================
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) {
----------------



================
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.
----------------
nit: how do you think about the suggested style? (not required)


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


================
Comment at: clang/lib/Sema/SemaLookup.cpp:3871-3873
+                DeclContext *Ctx = E->getDeclContext();
+                while (!Ctx->isFileContext() || Ctx->isInlineNamespace())
+                  Ctx = Ctx->getParent();
----------------
Maybe it is worth to implement `getNonInlineEnclosingNamespaceContext` and we could simplify the code here:


================
Comment at: clang/lib/Sema/SemaOverload.cpp:6407
+        getLangOpts().CPlusPlusModules &&
+        MF->getTopLevelModule() != getCurrentModule()->getTopLevelModule()) {
+      Candidate.Viable = false;
----------------
I introduced `isModuleUnitOfCurrentTU` to simplify the code a little bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129174