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