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

[Iain Sandoe via Phabricator via cfe-commits]

iains marked 2 inline comments as done.
iains added inline comments.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:6406
+    if (Function->getFormalLinkage() <= Linkage::InternalLinkage &&
+        getLangOpts().CPlusPlus20 && MF != getCurrentModule()) {
+      Candidate.Viable = false;
----------------
ChuanqiXu wrote:
> 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.
> 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/lib/Sema/SemaOverload.cpp:6406
+    if (Function->getFormalLinkage() <= Linkage::InternalLinkage &&
+        getLangOpts().CPlusPlus20 && MF != getCurrentModule()) {
+      Candidate.Viable = false;
----------------
iains wrote:
> ChuanqiXu wrote:
> > 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.
> > 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.
> 
> 
> The current implementation may reject following two cases:
> ```
> module;
> static void foo(int); // Ignore header
> ...

Actually, I have a note to check on the global module case, since we have special rules that allow merging of items there,


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

otherwise, agreed, these cases should be ok -  I guess we need a second test case with lookups that should succeed.



================
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"
----------------
ChuanqiXu wrote:
> 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.
well, it is related to the paper mentioned, but yes we can make this separate.



================
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;
----------------
ChuanqiXu wrote:
> This looks more consistent with the example.
This changes the example so that M is directly included in the implementation rather than transitively (the example does specifically use a different module name for the implementation)

I am not sure what you mean by "more consistent"
 (the example will fail to reject some of the lookups with this change).





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129174