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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

LGTM basically if the following comments addressed.



================
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.
----------------
iains wrote:
> 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,
> 
The current implementation skips a `break` of the outer loop.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129174