[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia marked an inline comment as done.
Anastasia added inline comments.


================
Comment at: clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl:521
+#else
+// expected-error at -5 {{assigning to '__generic int *__generic *' from incompatible type '__local int *__local *__private'}}
 #endif
----------------
rjmccall wrote:
> Can we specialize the diagnostic here so that we get the good diagnostic in both language modes?
I am not sure of the approach so I am wondering if you have any good suggestions.

It seems in C++ Clang exits with quite generic `Incompatible` in the implicit conversion case because the functionality for doing the semantic checks doesn't generally set `Sema::AssignConvertType`.

https://clang.llvm.org/doxygen/SemaExpr_8cpp_source.html

```

Sema::CheckSingleAssignmentConstraints:


  if (getLangOpts().CPlusPlus) {
      if (!LHSType->isRecordType() && !LHSType->isAtomicType()) {
        // C++ 5.17p3: If the left operand is not of class type, the
        // expression is implicitly converted (C++ 4) to the
        // cv-unqualified type of the left operand.
        QualType RHSType = RHS.get()->getType();
        if (Diagnose) {
          RHS = PerformImplicitConversion(RHS.get(), LHSType.getUnqualifiedType(),
                                          AA_Assigning);
        } else {
          ...
        }
        if (RHS.isInvalid())
          return Incompatible;
```

However for example in `IsStandardConversion`  I can see that `Sema::AssignConvertType` is being set but only for classifying the conversion sequence. It is never propagated outside.

http://clang.llvm.org/doxygen/SemaOverload_8cpp_source.html


```
IsStandardConversion:


    ExprResult ER = ExprResult{From};
    Sema::AssignConvertType Conv =
        S.CheckSingleAssignmentConstraints(ToType, ER,
                                           /*Diagnose=*/false,
                                           /*DiagnoseCFAudited=*/false,
                                           /*ConvertRHS=*/false);
    ImplicitConversionKind SecondConv;
    switch (Conv) {
    case Sema::Compatible:
      SecondConv = ICK_C_Only_Conversion;
      break;
    // For our purposes, discarding qualifiers is just as bad as using an
    // incompatible pointer. Note that an IncompatiblePointer conversion can drop
    // qualifiers, as well.
    case Sema::CompatiblePointerDiscardsQualifiers:
    case Sema::IncompatiblePointer:
    case Sema::IncompatiblePointerSign:
      SecondConv = ICK_Incompatible_Pointer_Conversion;
      break;
    default:
      return false;
    }


```

In most of other places it's not being used for C++ mode. I feel like C++ flow was not written to set the conversion type failure. Maybe there is something else I could use in C++ mode to specialize this diagnostic? Otherwise, it seems like quite a lot of Sema would have to be modified for C++ flow because implicit conversions are used in many different places.


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

https://reviews.llvm.org/D73360