[PATCH] D102850: [C++4OpenCL] Fix overloading resolution of addrspace constructors

[Anastasia Stulova via Phabricator via cfe-commits]

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