[PATCH] D129174: [C++20][Modules] Invalidate internal-linkage functions in overload sets [P1815R2 part 1]

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/include/clang/Sema/Overload.h:800
+    /// This candidate was not viable because it has internal linkage and is
+    /// from a different module than the use.
+    ovl_fail_module_mismatched,
----------------



================
Comment at: clang/lib/Sema/SemaOverload.cpp:6403
 
+  // Functions with internal linkage are only viable in the same module.
+  if (auto *MF = Function->getOwningModule()) {
----------------



================
Comment at: clang/lib/Sema/SemaOverload.cpp:6406
+    if (Function->getFormalLinkage() <= Linkage::InternalLinkage &&
+        getLangOpts().CPlusPlus20 && MF != getCurrentModule()) {
+      Candidate.Viable = false;
----------------
The current implementation may reject following two cases:
```
module;
static void foo(int); // Ignore header
...
export module A;
void bar() { foo(5); } // Should it be invalid?
```
and
```
export module A;
static void foo(int);
...
module :private;
void bar() { foo(5); } // Should it be invalid?
```

I mean in the above examples, the function of `foo(int)` is defined in the same TU but the call to it might be rejected.


================
Comment at: clang/test/CXX/basic/basic.link/p10-ex2.cpp:10-35
+//--- decls.h
+int f(); // #1, attached to the global module
+int g(); // #2, attached to the global module
+
+//--- M.cpp
+module;
+#include "decls.h"
----------------
I feel like the test is irrelevant with the revision, isn't it? If yes, I think we could land this as a separate NFC patch.


================
Comment at: clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p5-ex2.cpp:36-41
+//--- Q.cpp
+export module Q;
+
+//--- Q-impl.cpp
+module Q;
+import N;
----------------
This looks more consistent with the example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129174