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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
