[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/lib/Sema/SemaOverload.cpp:6406
+    if (Function->getFormalLinkage() <= Linkage::InternalLinkage &&
+        getLangOpts().CPlusPlus20 && MF != getCurrentModule()) {
+      Candidate.Viable = false;
----------------
iains wrote:
> 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.
> 
> Actually, I have a note to check on the global module case, since we have special rules that allow merging of items there,

Yeah, it is surprising that we couldn't handle the global module fragment case.


================
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;
----------------
iains wrote:
> 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).
> 
> 
> 
Oh, I didn't notice it uses a different module name. My bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129174