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)) {
----------------
Anastasia wrote:
> 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?
ok, let's also add a comment explaining that ctors are handled separately from
the other members because they don't have an object parameter created
explicitly (if that is the reason).
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
