Anastasia added inline comments.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:9870
+ if (S.getLangOpts().OpenCL) {
+ if (const auto *CD1 =
dyn_cast_or_null<CXXConstructorDecl>(Cand1.Function)) {
----------------
olestrohm wrote:
> Anastasia wrote:
> > olestrohm wrote:
> > > Anastasia wrote:
> > > > olestrohm wrote:
> > > > > Anastasia wrote:
> > > > > > I think we should remove the OpenCL check since it is not OpenCL
> > > > > > specific rule and you are using common helpers indeed!
> > > > > >
> > > > > > I also wonder if this should be applied to all member functions not
> > > > > > only ctors since `isBetterOverloadCandidate` should be used for
> > > > > > everything?
> > > > > >
> > > > > > However it seems that other members are already handled somehow
> > > > > > https://godbolt.org/z/MrWKPKed7. Do you know where this handling
> > > > > > comes from?
> > > > > It's handled in SemaOverload.cpp:10000 in
> > > > > `OverloadCandidateSet::BestViableFunction` which checks if the given
> > > > > function is viable or not. For member functions the address space
> > > > > they're defined in and the address space of the variable have to
> > > > > match. Therefore only one of them is valid at a time and they never
> > > > > get checked against each other in `isBetterOverloadCandidate`.
> > > > I am quite sure that for the example above both `__private` and
> > > > `__generic `overloads are viable. In fact, if you remove the
> > > > `__private` overload the `__generic` one gets picked.
> > > >
> > > > So I think the logic should be checking for compatibility rather than
> > > > the exact match i.e. using `isAddressSpaceSupersetOf` although it might
> > > > not be calling that helper directly but through the other Qualifier
> > > > logic.
> > > Ah, I think I checked it wrong last time. Going through it again it's
> > > actually determined by `QualType::isMoreQualifiedThan` in the end. I'll
> > > look into it more.
> > So did you happen to find where the address space ranking for the members
> > is currently happening for this test case https://godbolt.org/z/MrWKPKed7?
> >
> > I just want to make sure we don't add any redundant checks... so if let's
> > say we already have a similar logic elsewhere it would be better if we
> > attempt to generalize it or move it entirely here if it's doable. In
> > general, we should try to use general qualifier logic if it works instead
> > of specializing for address spaces.
> It's handled on line 9661 with a call to CompareImplicitConversionSequences,
> which then goes through a couple of checks and discovers that the return type
> of one of the functions is more specialized than the other. However this uses
> the Conversions part of the candidates, which the constructors don't have.
>
> I did attempt to generalize this further than address spaces, but that caused
> some tests to fail, so I think it makes sense to only check address spaces
> here.
Ah I see. Is it because the object parameter is not added implicitly in the
ctors?
If this is only working for ctors I wonder if we should cast to
`CXXConstructorDecl` instead of `CXXMethodDecl` or/and also add a comment
explaining that?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102850/new/
https://reviews.llvm.org/D102850
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
