[PATCH] D56735: [OpenCL] Fix overloading ranking rules to work correctly for address space coversions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D56735#1358222 <https://reviews.llvm.org/D56735#1358222>, @rjmccall wrote:

> Is there a reason not to just do `T1.getQualifiers() == T2.getQualifiers()`?


I tried this but it causes ObjC test to fail test/SemaObjCXX/arc-overloading.mm.

  38 // Simple overloading
  39 int &f2(id __strong const *); // expected-note{{candidate function}}
  40 float &f2(id __autoreleasing const *); // expected-note{{candidate function}}
  41 
  42 void test_f2() {
  43   id __strong *sip;
  44   id __strong const *csip;
  45   id __weak *wip;
  46   id __autoreleasing *aip;
  47   id __unsafe_unretained *uip;
  48 
  49   // Prefer non-ownership conversions to ownership conversions.
  50   int &ir1 = f2(sip);
  51   int &ir2 = f2(csip);
  52   float &fr1 = f2(aip);
  53 
  54   f2(uip); // expected-error{{call to 'f2' is ambiguous}}
  55 }

Clang fails to resolve the overloads:

  error: 'error' diagnostics seen but not expected: 
    File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 50: call to 'f2' is ambiguous
    File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 52: call to 'f2' is ambiguous
  error: 'note' diagnostics seen but not expected: 
    File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 39: candidate function
    File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 40: candidate function
    File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 39: candidate function
    File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 40: candidate function

Since CVR quals for both overloads of `f2` are the same it didn't continue ranking the other qualifiers before. But now when it continues looking at the other qualifiers it finds that `__strong` and `__autoreleasing` are disjoint and exits at the following return statement in `CompareQualificationConversions`:

  } else {
    // Qualifiers are disjoint.
    return ImplicitConversionSequence::Indistinguishable;
  }

I feel the logic here doesn't apply to this case because it doesn't take into account that for the passed in function argument there is no ownership qualification conversion for one of the overloads. But I don't have a good idea how to fix this easily other than just keep fast pathing  ObjC qual here.

May be if we add a separate qualification conversion for ObjC ownership qualification only we will end up with more conversion steps for one overload and therefore it will be possible to resolve the ambiguity? Do you have any other suggestions or do you think someone else could help?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56735/new/

https://reviews.llvm.org/D56735