[clang] p0388 conversion to incomplete array

[urnathan]

Summary

This implements the new implicit conversion sequence to an incomplete (unbounded) array type. It is mostly Richard Smith's work (from D102650), updated to trunk, testcases added and a few bugs fixed found in such testing.

It is not a complete implementation of p0388.

[rsmith]

clang/include/clang/Sema/Initialization.h

1288 ↗

(On Diff #345963)

clang/lib/AST/ExprConstant.cpp

7799 ↗

(On Diff #345963)

Are there any cases here where we're adding an array bound and we need to check that it's correct? Presumably casts that add an array bound can result in wrongly-typed pointers during constant evaluation, but they'll only be wrong by virtue of having the wrong array bounds, and we should be using the stored value rather than the pointer type for our bounds checks, so maybe that's OK?

13203 ↗

(On Diff #345963)

I'd be inclined to add this to the above "invalid cast kind for integral value" list instead, since we have such a list here.

13935 ↗

(On Diff #345963)

Similarly, this is an invalid cast kind for complex so we could move this to the previous set of cases.

clang/lib/CodeGen/CGExpr.cpp

4691–4692

Instead of manually preserving the address space, you could compute it from the destination reference type, like we do for CK_LValueBitCast below.

4759

I think you will need to do this too, in case the destination type is a variably-modified type. Eg, in:

extern int arr[];
int g();
void f() {
  return ((int(&)[g()])arr)[0];
}

... we need to emit the call to g().

clang/lib/CodeGen/CGExprAgg.cpp

861 ↗

(On Diff #345963)

Hm. Aggregate IR emission can be used for lvalues of aggregate types (IR generation will step through temporary materialization and lvalue-to-rvalue conversions looking for the source of the value, to optimize away some obviously-unnecessary copies). But... I think this is still impossible, because array lvalues are never used to initialize arrays.

clang/lib/CodeGen/CGExprConstant.cpp

1129 ↗

(On Diff #345963)

(due to your changes to ExprConstant.cpp, yes)

clang/lib/Sema/SemaExprCXX.cpp

4607–4608

Do we need to handle the case where there's both an address-space change and an array-bound change? I'd imagine we want two ImpCastExprs in that case, with a suitably-determined intermediate type (either the source type with the destination address space or the destination type with the source address space; I can't think of a good reason to prefer one over the other).

clang/lib/Sema/SemaInit.cpp

8398 ↗

(On Diff #345963)

We may need to preserve the value kind, like we do for QualificationConversion. We could use the value kind of CurInit.get(), but I don't think that will do the right thing for initializing a const lvalue reference from an array xvalue:

using T = int[3];
const int (&arr)[] = T{1, 2, 3}; // materialized temporary is an xvalue, but converted initializer should be an lvalue

clang/lib/Sema/SemaOverload.cpp

292

Yes, this seems right to me. We're only trying to strip off casts that either might be, or might follow, a narrowing conversion (to get to the pre-narrowing type), and an array bound conversion can't be either.

3280

It'd be consistent with ObjCLifetimeConversion to use a reference here. I mean, in an ideal world I'd prefer we didn't have three bool parameters in a row, especially not with two of them being out-parameters. Maybe we could return a bitfield enum, eg:

enum QualificationConversionKind {
  QCK_NotQualificationConversion = 0x0,
  QCK_QualificationConversion = 0x1,
  QCK_IncludesObjCLifetimeConversion = 0x2,
  QCK_IncludesArrayBoundConversion = 0x4
};

3294–3306

Style nits:

• we generally use while (true) not for (;;)
• please use braces for ifs containing (transitively) more than one statement

4251

Yes, I think we do need a comparison here.

I made a start on P0388 a couple of years ago. I never got very far, but it looks like the parts I covered are mostly non-overlapping with the parts you've covered, and do include this. I've uploaded my work here: https://reviews.llvm.org/D102650; please feel free to take all / some / none of that :)

4580

Yeah, it should be correct to isa / cast directly.

[urnathan]

Richard, thanks for the comments -- I'd not realized I'd left quite so many FIXMEs lying around! The high bit I'm taking is that you're fine with the approach I'm taking.

clang/lib/AST/ExprConstant.cpp

7799 ↗

(On Diff #345963)

This patch only allows the bound->unbound (even though the code emission is more general), but yes, when unbound->bound is permitted we could end up with such invalid constant evaluations. Making sure that DTRT is on the ToDo list.

clang/lib/CodeGen/CGExpr.cpp

4691–4692

Sounds good.

clang/lib/Sema/SemaExprCXX.cpp

4607–4608

Ok, The code emission for ArrayBound will handle changing the address space, but a 2 step AST makes sense. I think I have a slight preference for placing the address-space change outermost.

Question: Is the address space change only concerning the outermost pointer? What about such changes that appear in the middle of a cv-decomposition?

clang/lib/Sema/SemaOverload.cpp

3280

was wondering about that too. Generally I dislike such out parameters and a bitflag return would be nicer. Presumably with a rename from IsFOO to FOOKind or similar?

3294–3306

thanks for guiding.

Question: What about collating blocks of cascading case labels? They seem to be unordered, which does make navigating them unclear.

[urnathan]

Cleanup diff, I forgot to review the actual diff :(

[urnathan]

ping?

[rsmith]

If you want to skip the VLA handling for now, that's fine by me, but I think we do eventually want to allow this kind of conversion for VLAs too, and it looks like it might be pretty straightforward to do as part of this change. I think we should at least handle the composite pointer type case for two constant arrays here, though. Eg:

int (*a)[3];
int (*b)[4];
auto *p = true ? a : b; // should be int (*)[] in C++20

clang/lib/AST/ASTContext.cpp

5865

(A little more VLA work here.)

clang/lib/Sema/SemaExprCXX.cpp

4607–4608

I think a non-top-level address space change would necessarily be a reinterpret cast: a T AS(1) * and a T AS(2) * might have different representations, so a T AS(1) ** cannot in general be meaningfully cast to T AS(2) **. (I don't think we currently special-case the situation where the pointer representations are the same between address spaces for such casts, but I could be wrong about that.)

6730

We seem to not handle the case where both types are constant array types, but with different bounds (where, based on the comment below, we're supposed to form an array of unknown bound). We should probably also unwrap here if one of the types is an incomplete array type and the other is a VLA. And maybe also if we have two distinct VLA types, or a VLA and a constant-bound array. So I think perhaps the logic should be:

• Prior to C++20: if both types to be incomplete array types, unwrap and add an incomplete array step.
• In C++20 onwards: if we get this far (if we have two array types that don't have the same constant bound) and the types aren't equal, unwrap and add an incomplete array step.

clang/lib/Sema/SemaOverload.cpp

3258

I think we should also allow VLA -> array of unknown bound conversions as qualification conversions.

[urnathan]

This adds support for the cond ? arya : aryb cases you pointed out. There is a problem with decaying VLAs to arrays of unknown bound -- Implicit Conversion Sequence was unprepared to deal with that. It's starting to look a little rat-holey, so if you don't mind I've not handled that.

[urnathan]

clang/lib/Sema/SemaExprCXX.cpp

6730

Handling VLA -> [] fell into problems with Implicit Conversion Sequence, so I punted that. Constant array -> [] is handled though.

[urnathan]

I've marked the comments from the v1 patch as addressed, but most of them were mooted by the v2 patch based on your D102650 patch. I find Phabricator a little hard to use :(

[urnathan]

ping? as commented, the v2 patch's absorption of Richard's D102650 diff mooted many of the original comments.

[urnathan]

ping?

[aaron.ballman]

Can you also update the cxx_status page to show that we're getting a partial implementation of P0388?

The changes LGTM, though I spotted an unrelated change that can go in as an NFC commit while we finalize this review. I'm adding Erich as a second set of eyes, in case Richard is busy at the moment.

clang/lib/Sema/SemaInit.cpp

8255 ↗

(On Diff #355943)

Unrelated typo fix (feel free to land as an NFC change though).

[urnathan]

the cxx_status change is in https://reviews.llvm.org/D103908, which is ready to land once this one is done :) Then you get the whole of p0388.

[urnathan]

ping?

[aaron.ballman]

LGTM!

clang/include/clang/AST/ASTContext.h

if (SubTnullability.hasValue() == SuperTnullability.hasValue()) {
               *SubTnullability == NullabilityKind::NonNull);
     }
     return true;
   }
 
   bool ObjCMethodsAreEqual(const ObjCMethodDecl *MethodDecl,
                            const ObjCMethodDecl *MethodImp);
 
-  bool UnwrapSimilarTypes(QualType &T1, QualType &T2);
-  void UnwrapSimilarArrayTypes(QualType &T1, QualType &T2);
+  bool UnwrapSimilarTypes(QualType &T1, QualType &T2,
+                          bool AllowPiMismatch = true);
+  void UnwrapSimilarArrayTypes(QualType &T1, QualType &T2,
+                               bool AllowPiMismatch = true);
 
   /// Determine if two types are similar, according to the C++ rules. That is,
   /// determine if they are the same other than qualifiers on the initial
   /// sequence of pointer / pointer-to-member / array (and in Clang, object
   /// pointer) types and their element types.
   ///
   /// Clang offers a number of qualifiers in addition to the C++ qualifiers;
   /// those qualifiers are also ignored in the 'similarity' check.

clang/lib/AST/ASTContext.cpp

QualType ASTContext::getUnqualifiedArrayType(QualType type,
   return getDependentSizedArrayType(unqualElementType, DSAT->getSizeExpr(),
                                     DSAT->getSizeModifier(), 0,
                                     SourceRange());
 }
 
 /// Attempt to unwrap two types that may both be array types with the same bound
 /// (or both be array types of unknown bound) for the purpose of comparing the
 /// cv-decomposition of two types per C++ [conv.qual].
-void ASTContext::UnwrapSimilarArrayTypes(QualType &T1, QualType &T2) {
+///
+/// \param AllowPiMismatch Allow the Pi1 and Pi2 to differ as described in
+///        C++20 [conv.qual], if permitted by the current language mode.
+void ASTContext::UnwrapSimilarArrayTypes(QualType &T1, QualType &T2,
+                                         bool AllowPiMismatch) {
   while (true) {
     auto *AT1 = getAsArrayType(T1);
     if (!AT1)
       return;
 
     auto *AT2 = getAsArrayType(T2);
     if (!AT2)
       return;
 
     // If we don't have two array types with the same constant bound nor two
     // incomplete array types, we've unwrapped everything we can.
+    // C++20 also permits one type to be a constant array type and the other
+    // to be an incomplete array type.
+    // FIXME: Consider also unwrapping array of unknown bound and VLA.
     if (auto *CAT1 = dyn_cast<ConstantArrayType>(AT1)) {
       auto *CAT2 = dyn_cast<ConstantArrayType>(AT2);
-      if (!CAT2 || CAT1->getSize() != CAT2->getSize())
+      if (!(CAT2 && CAT1->getSize() == CAT2->getSize()) &&
+          !(getLangOpts().CPlusPlus20 && AllowPiMismatch &&
+            isa<IncompleteArrayType>(AT2)))
+        return;
+    } else if (isa<IncompleteArrayType>(AT1)) {
+      if (!isa<IncompleteArrayType>(AT2) &&
+          !(getLangOpts().CPlusPlus20 && AllowPiMismatch &&
+            isa<ConstantArrayType>(AT2)))
         return;
-    } else if (!isa<IncompleteArrayType>(AT1) ||
-               !isa<IncompleteArrayType>(AT2)) {
+    } else {
       return;
     }
 
     T1 = AT1->getElementType();
     T2 = AT2->getElementType();
   }
 }
 
 /// Attempt to unwrap two types that may be similar (C++ [conv.qual]).
 ///
 /// If T1 and T2 are both pointer types of the same kind, or both array types
 /// with the same bound, unwraps layers from T1 and T2 until a pointer type is
 /// unwrapped. Top-level qualifiers on T1 and T2 are ignored.
 ///
 /// This function will typically be called in a loop that successively
 /// "unwraps" pointer and pointer-to-member types to compare them at each
 /// level.
 ///
+/// \param AllowPiMismatch Allow the Pi1 and Pi2 to differ as described in
+///        C++20 [conv.qual], if permitted by the current language mode.
+///
 /// \return \c true if a pointer type was unwrapped, \c false if we reached a
 /// pair of types that can't be unwrapped further.
-bool ASTContext::UnwrapSimilarTypes(QualType &T1, QualType &T2) {
-  UnwrapSimilarArrayTypes(T1, T2);
+bool ASTContext::UnwrapSimilarTypes(QualType &T1, QualType &T2,
+                                    bool AllowPiMismatch) {
+  UnwrapSimilarArrayTypes(T1, T2, AllowPiMismatch);
 
   const auto *T1PtrType = T1->getAs<PointerType>();
   const auto *T2PtrType = T2->getAs<PointerType>();
   if (T1PtrType && T2PtrType) {
     T1 = T1PtrType->getPointeeType();
     T2 = T2PtrType->getPointeeType();
     return true;
   }
while (true) {
     Quals1.removeCVRQualifiers();
     Quals2.removeCVRQualifiers();
     if (Quals1 != Quals2)
       return false;
 
     if (hasSameType(T1, T2))
       return true;
 
-    if (!UnwrapSimilarTypes(T1, T2))
+    if (!UnwrapSimilarTypes(T1, T2, /*AllowPiMismatch*/ false))
       return false;
   }
 }
 
 DeclarationNameInfo
 ASTContext::getNameForTemplate(TemplateName Name,
                                SourceLocation NameLoc) const {
   switch (Name.getKind()) {

clang/lib/CodeGen/CGExpr.cpp

case CK_Dynamic: {
     const auto *DCE = cast<CXXDynamicCastExpr>(E);
     return MakeNaturalAlignAddrLValue(EmitDynamicCast(V, DCE), E->getType());
   }
 
   case CK_ConstructorConversion:
   case CK_UserDefinedConversion:
   case CK_CPointerToObjCPointerCast:
   case CK_BlockPointerToObjCPointerCast:
-  case CK_NoOp:
   case CK_LValueToRValue:
     return EmitLValue(E->getSubExpr());
 
+  case CK_NoOp: {
+    // CK_NoOp can model a qualification conversion, which can remove an array
+    // bound and change the IR type.
+    // FIXME: Once pointee types are removed from IR, remove this.
+    LValue LV = EmitLValue(E->getSubExpr());
+    if (LV.isSimple()) {
+      Address V = LV.getAddress(*this);
+      if (V.isValid()) {
+        llvm::Type *T =
+            ConvertTypeForMem(E->getType())
+                ->getPointerTo(
+                    cast<llvm::PointerType>(V.getType())->getAddressSpace());
+        if (V.getType() != T)
+          LV.setAddress(Builder.CreateBitCast(V, T));
+      }
+    }
+    return LV;
+  }
+
   case CK_UncheckedDerivedToBase:
   case CK_DerivedToBase: {
     const auto *DerivedClassTy =
         E->getSubExpr()->getType()->castAs<RecordType>();
     auto *DerivedClassDecl = cast<CXXRecordDecl>(DerivedClassTy->getDecl());
 
     LValue LV = EmitLValue(E->getSubExpr());
     Address This = LV.getAddress(*this);
case CK_BaseToDerived: {
 
     return MakeAddrLValue(Derived, E->getType(), LV.getBaseInfo(),
                           CGM.getTBAAInfoForSubobject(LV, E->getType()));
   }
   case CK_LValueBitCast: {
     // This must be a reinterpret_cast (or c-style equivalent).
     const auto *CE = cast<ExplicitCastExpr>(E);
 
     CGM.EmitExplicitCastExprType(CE, this);
     LValue LV = EmitLValue(E->getSubExpr());
     Address V = Builder.CreateBitCast(LV.getAddress(*this),
                                       ConvertType(CE->getTypeAsWritten()));
 
     if (SanOpts.has(SanitizerKind::CFIUnrelatedCast))
       EmitVTablePtrCheckForCast(E->getType(), V.getPointer(),
                                 /*MayBeNull=*/false, CFITCK_UnrelatedCast,
                                 E->getBeginLoc());

clang/lib/CodeGen/CGExprScalar.cpp

case CK_AddressSpaceConversion: {
     // Since target may map different address spaces in AST to the same address
     // space, an address space conversion may end up as a bitcast.
     return CGF.CGM.getTargetCodeGenInfo().performAddrSpaceCast(
         CGF, Visit(E), E->getType()->getPointeeType().getAddressSpace(),
         DestTy->getPointeeType().getAddressSpace(), ConvertType(DestTy));
   }
   case CK_AtomicToNonAtomic:
   case CK_NonAtomicToAtomic:
-  case CK_NoOp:
   case CK_UserDefinedConversion:
     return Visit(const_cast<Expr*>(E));
 
+  case CK_NoOp: {
+    llvm::Value *V = Visit(const_cast<Expr *>(E));
+    if (V) {
+      // CK_NoOp can model a pointer qualification conversion, which can remove
+      // an array bound and change the IR type.
+      // FIXME: Once pointee types are removed from IR, remove this.
+      llvm::Type *T = ConvertType(DestTy);
+      if (T != V->getType())
+        V = Builder.CreateBitCast(V, T);
+    }
+    return V;
+  }
+
   case CK_BaseToDerived: {
     const CXXRecordDecl *DerivedClassDecl = DestTy->getPointeeCXXRecordDecl();
     assert(DerivedClassDecl && "BaseToDerived arg isn't a C++ object pointer!");
 
     Address Base = CGF.EmitPointerWithAlignment(E);
     Address Derived =
       CGF.GetAddressOfDerivedClass(Base, DerivedClassDecl,
                                    CE->path_begin(), CE->path_end(),

clang/lib/CodeGen/CGObjC.cpp

else
       argCK = CK_CPointerToObjCPointerCast;
   } else if (ivarRef.getType()->isBlockPointerType()) {
      if (argLoad.getType()->isBlockPointerType())
       argCK = CK_BitCast;
     else
       argCK = CK_AnyPointerToBlockPointerCast;
   } else if (ivarRef.getType()->isPointerType()) {
     argCK = CK_BitCast;
+  } else if (argLoad.getType()->isAtomicType() &&
+             !ivarRef.getType()->isAtomicType()) {
+    argCK = CK_AtomicToNonAtomic;
+  } else if (!argLoad.getType()->isAtomicType() &&
+             ivarRef.getType()->isAtomicType()) {
+    argCK = CK_NonAtomicToAtomic;
   }
   ImplicitCastExpr argCast(ImplicitCastExpr::OnStack, ivarRef.getType(), argCK,
                            &argLoad, VK_PRValue, FPOptionsOverride());
   Expr *finalArg = &argLoad;
   if (!getContext().hasSameUnqualifiedType(ivarRef.getType(),
                                            argLoad.getType()))
     finalArg = &argCast;
 

clang/lib/Sema/SemaCast.cpp

if (SrcExpr.isInvalid())
     return TC_Failed;
   if (tcr != TC_NotApplicable)
     return tcr;
 
   // C++ 5.2.9p6: May apply the reverse of any standard conversion, except
   // lvalue-to-rvalue, array-to-pointer, function-to-pointer, and boolean
   // conversions, subject to further restrictions.
   // Also, C++ 5.2.9p1 forbids casting away constness, which makes reversal
-  // of qualification conversions impossible.
+  // of qualification conversions impossible. (In C++20, adding an array bound
+  // would be the reverse of a qualification conversion, but adding permission
+  // to add an array bound in a static_cast is a wording oversight.)
   // In the CStyle case, the earlier attempt to const_cast should have taken
   // care of reverse qualification conversions.
 
   QualType SrcType = Self.Context.getCanonicalType(SrcExpr.get()->getType());
 
   // C++0x 5.2.9p9: A value of a scoped enumeration type can be explicitly
   // converted to an integral type. [...] A value of a scoped enumeration type
   // can also be explicitly converted to a floating-point type [...].

clang/lib/Sema/SemaExprCXX.cpp

if (ToType->isReferenceType() &&
             From->getType().getAddressSpace())
       CK = CK_AddressSpaceConversion;
 
     if (ToType->isPointerType() &&
         ToType->getPointeeType().getAddressSpace() !=
             From->getType()->getPointeeType().getAddressSpace())
       CK = CK_AddressSpaceConversion;
 
     From = ImpCastExprToType(From, ToType.getNonLValueExprType(Context), CK, VK,
                              /*BasePath=*/nullptr, CCK)
                .get();
 
     if (SCS.DeprecatedStringLiteralToCharPtr &&
         !getLangOpts().WritableStrings) {
       Diag(From->getBeginLoc(),
            getLangOpts().CPlusPlus11
                ? diag::ext_deprecated_string_literal_conversion
                : diag::warn_deprecated_string_literal_conversion)
if (!Steps.empty()) {
         return QualType();
 
       Steps.back().Quals = Quals;
       if (Q1 != Quals || Q2 != Quals)
         NeedConstBefore = Steps.size() - 1;
     }
 
     // FIXME: Can we unify the following with UnwrapSimilarTypes?
+
+    const ArrayType *Arr1, *Arr2;
+    if ((Arr1 = Context.getAsArrayType(Composite1)) &&
+        (Arr2 = Context.getAsArrayType(Composite2))) {
+      auto *CAT1 = dyn_cast<ConstantArrayType>(Arr1);
+      auto *CAT2 = dyn_cast<ConstantArrayType>(Arr2);
+      if (CAT1 && CAT2 && CAT1->getSize() == CAT2->getSize()) {
+        Composite1 = Arr1->getElementType();
+        Composite2 = Arr2->getElementType();
+        Steps.emplace_back(Step::Array, CAT1);
+        continue;
+      }
+      bool IAT1 = isa<IncompleteArrayType>(Arr1);
+      bool IAT2 = isa<IncompleteArrayType>(Arr2);
+      if ((IAT1 && IAT2) ||
+          (getLangOpts().CPlusPlus20 && (IAT1 != IAT2) &&
+           ((bool)CAT1 != (bool)CAT2) &&
+           (Steps.empty() || Steps.back().K != Step::Array))) {
+        // In C++20 onwards, we can unify an array of N T with an array of
+        // a different or unknown bound. But we can't form an array whose
+        // element type is an array of unknown bound by doing so.
+        Composite1 = Arr1->getElementType();
+        Composite2 = Arr2->getElementType();
+        Steps.emplace_back(Step::Array);
+        if (CAT1 || CAT2)
+          NeedConstBefore = Steps.size();
+        continue;
+      }
+    }
+
     const PointerType *Ptr1, *Ptr2;
     if ((Ptr1 = Composite1->getAs<PointerType>()) &&
         (Ptr2 = Composite2->getAs<PointerType>())) {
       Composite1 = Ptr1->getPointeeType();
       Composite2 = Ptr2->getPointeeType();
       Steps.emplace_back(Step::Pointer);
       continue;
     }
if (Steps.empty() && ((Composite1->isVoidPointerType() &&
                           (Composite1->isObjCObjectPointerType() &&
                            Composite2->isVoidPointerType()))) {
       Composite1 = Composite1->getPointeeType();
       Composite2 = Composite2->getPointeeType();
       Steps.emplace_back(Step::Pointer);
       continue;
     }
 
-    // FIXME: arrays
-
     // FIXME: block pointer types?
 
     // Cannot unwrap any more types.
     break;
   }
 
   //  - if T1 or T2 is "pointer to noexcept function" and the other type is
   //    "pointer to function", where the function types are otherwise the same,

clang/lib/Sema/SemaOverload.cpp

while (auto *ICE = dyn_cast<ImplicitCastExpr>(Converted)) {
     case CK_IntegralToFloating:
     case CK_BooleanToSignedIntegral:
     case CK_FloatingToIntegral:
     case CK_FloatingToBoolean:
     case CK_FloatingCast:
       Converted = ICE->getSubExpr();
       continue;
 
     default:
       return Converted;
     }
   }
 
   return Converted;
 }
 
 /// Check if this standard conversion sequence represents a narrowing
if (ToQuals.getAddressSpace() != FromQuals.getAddressSpace() &&
     return false;
 
   //   -- if the cv 1,j and cv 2,j are different, then const is in
   //      every cv for 0 < k < j.
   if (!CStyle && FromQuals.getCVRQualifiers() != ToQuals.getCVRQualifiers() &&
       !PreviousToQualsIncludeConst)
     return false;
 
+  // The following wording is from C++20, where the result of the conversion
+  // is T3, not T2.
+  //   -- if [...] P1,i [...] is "array of unknown bound of", P3,i is
+  //      "array of unknown bound of"
+  if (FromType->isIncompleteArrayType() && !ToType->isIncompleteArrayType())
+    return false;
+
+  //   -- if the resulting P3,i is different from P1,i [...], then const is
+  //      added to every cv 3_k for 0 < k < i.
+  if (!CStyle && FromType->isConstantArrayType() &&
+      ToType->isIncompleteArrayType() && !PreviousToQualsIncludeConst)
+    return false;
+
   // Keep track of whether all prior cv-qualifiers in the "to" type
   // include const.
   PreviousToQualsIncludeConst =
       PreviousToQualsIncludeConst && ToQuals.hasConst();
   return true;
 }
 
 /// IsQualificationConversion - Determines whether the conversion from
 /// an rvalue of type FromType to ToType is a qualification conversion
 /// (C++ 4.4).
 ///
 /// \param ObjCLifetimeConversion Output parameter that will be set to indicate
 /// when the qualification conversion involves a change in the Objective-C
 /// object lifetime.
 bool
 Sema::IsQualificationConversion(QualType FromType, QualType ToType,
                                 bool CStyle, bool &ObjCLifetimeConversion) {
   FromType = Context.getCanonicalType(FromType);
   ToType = Context.getCanonicalType(ToType);
   ObjCLifetimeConversion = false;
 
   // If FromType and ToType are the same type, this is not a
   // qualification conversion.
   if (FromType.getUnqualifiedType() == ToType.getUnqualifiedType())
     return false;
 
   // (C++ 4.4p4):
   //   A conversion can add cv-qualifiers at levels other than the first
   //   in multi-level pointers, subject to the following rules: [...]
   bool PreviousToQualsIncludeConst = true;
   bool UnwrappedAnyPointer = false;
   while (Context.UnwrapSimilarTypes(FromType, ToType)) {
     if (!isQualificationConversionStep(
             FromType, ToType, CStyle, !UnwrappedAnyPointer,
             PreviousToQualsIncludeConst, ObjCLifetimeConversion))
       return false;
     UnwrappedAnyPointer = true;
   }
 
   // We are left with FromType and ToType being the pointee types
   // after unwrapping the original FromType and ToType the same number
   // of times. If we unwrapped any pointers, and if FromType and
   // ToType have the same unqualified type (since we checked
   // qualifiers above), then this is a qualification conversion.
   return UnwrappedAnyPointer && Context.hasSameUnqualifiedType(FromType,ToType);
 }
 
 /// - Determine whether this is a conversion from a scalar type to an
 /// atomic type.
 ///
 /// If successful, updates \c SCS's second and third steps in the conversion
 /// sequence to finish the conversion.
 static bool tryAtomicConversion(Sema &S, Expr *From, QualType ToType,

 
 /// CompareQualificationConversions - Compares two standard conversion
 /// sequences to determine whether they can be ranked based on their
 /// qualification conversions (C++ 13.3.3.2p3 bullet 3).
 static ImplicitConversionSequence::CompareKind
 CompareQualificationConversions(Sema &S,
                                 const StandardConversionSequence& SCS1,
                                 const StandardConversionSequence& SCS2) {
-  // C++ 13.3.3.2p3:
+  // C++ [over.ics.rank]p3:
   //  -- S1 and S2 differ only in their qualification conversion and
-  //     yield similar types T1 and T2 (C++ 4.4), respectively, and the
-  //     cv-qualification signature of type T1 is a proper subset of
-  //     the cv-qualification signature of type T2, and S1 is not the
+  //     yield similar types T1 and T2 (C++ 4.4), respectively, [...]
+  // [C++98]
+  //     [...] and the cv-qualification signature of type T1 is a proper subset
+  //     of the cv-qualification signature of type T2, and S1 is not the
   //     deprecated string literal array-to-pointer conversion (4.2).
+  // [C++2a]
+  //     [...] where T1 can be converted to T2 by a qualification conversion.
   if (SCS1.First != SCS2.First || SCS1.Second != SCS2.Second ||
       SCS1.Third != SCS2.Third || SCS1.Third != ICK_Qualification)
     return ImplicitConversionSequence::Indistinguishable;
 
   // FIXME: the example in the standard doesn't use a qualification
   // conversion (!)
   QualType T1 = SCS1.getToType(2);
   QualType T2 = SCS2.getToType(2);
   T1 = S.Context.getCanonicalType(T1);
   T2 = S.Context.getCanonicalType(T2);
   assert(!T1->isReferenceType() && !T2->isReferenceType());
   Qualifiers T1Quals, T2Quals;
   QualType UnqualT1 = S.Context.getUnqualifiedArrayType(T1, T1Quals);
   QualType UnqualT2 = S.Context.getUnqualifiedArrayType(T2, T2Quals);
 
   // If the types are the same, we won't learn anything by unwrapping
   // them.
   if (UnqualT1 == UnqualT2)
     return ImplicitConversionSequence::Indistinguishable;
 
-  ImplicitConversionSequence::CompareKind Result
-    = ImplicitConversionSequence::Indistinguishable;
+  // Don't ever prefer a standard conversion sequence that uses the deprecated
+  // string literal array to pointer conversion.
+  bool CanPick1 = !SCS1.DeprecatedStringLiteralToCharPtr;
+  bool CanPick2 = !SCS2.DeprecatedStringLiteralToCharPtr;
 
   // Objective-C++ ARC:
   //   Prefer qualification conversions not involving a change in lifetime
-  //   to qualification conversions that do not change lifetime.
-  if (SCS1.QualificationIncludesObjCLifetime !=
-                                      SCS2.QualificationIncludesObjCLifetime) {
-    Result = SCS1.QualificationIncludesObjCLifetime
-               ? ImplicitConversionSequence::Worse
-               : ImplicitConversionSequence::Better;
-  }
+  //   to qualification conversions that do change lifetime.
+  if (SCS1.QualificationIncludesObjCLifetime &&
+      !SCS2.QualificationIncludesObjCLifetime)
+    CanPick1 = false;
+  if (SCS2.QualificationIncludesObjCLifetime &&
+      !SCS1.QualificationIncludesObjCLifetime)
+    CanPick2 = false;
 
-  while (S.Context.UnwrapSimilarTypes(T1, T2)) {
-    // Within each iteration of the loop, we check the qualifiers to
-    // determine if this still looks like a qualification
-    // conversion. Then, if all is well, we unwrap one more level of
-    // pointers or pointers-to-members and do it all again
-    // until there are no more pointers or pointers-to-members left
-    // to unwrap. This essentially mimics what
-    // IsQualificationConversion does, but here we're checking for a
-    // strict subset of qualifiers.
-    if (T1.getQualifiers().withoutObjCLifetime() ==
-        T2.getQualifiers().withoutObjCLifetime())
-      // The qualifiers are the same, so this doesn't tell us anything
-      // about how the sequences rank.
-      // ObjC ownership quals are omitted above as they interfere with
-      // the ARC overload rule.
-      ;
-    else if (T2.isMoreQualifiedThan(T1)) {
-      // T1 has fewer qualifiers, so it could be the better sequence.
-      if (Result == ImplicitConversionSequence::Worse)
-        // Neither has qualifiers that are a subset of the other's
-        // qualifiers.
-        return ImplicitConversionSequence::Indistinguishable;
-
-      Result = ImplicitConversionSequence::Better;
-    } else if (T1.isMoreQualifiedThan(T2)) {
-      // T2 has fewer qualifiers, so it could be the better sequence.
-      if (Result == ImplicitConversionSequence::Better)
-        // Neither has qualifiers that are a subset of the other's
-        // qualifiers.
-        return ImplicitConversionSequence::Indistinguishable;
+  bool ObjCLifetimeConversion;
+  if (CanPick1 &&
+      !S.IsQualificationConversion(T1, T2, false, ObjCLifetimeConversion))
+    CanPick1 = false;
+  // FIXME: In Objective-C ARC, we can have qualification conversions in both
+  // directions, so we can't short-cut this second check in general.
+  if (CanPick2 &&
+      !S.IsQualificationConversion(T2, T1, false, ObjCLifetimeConversion))
+    CanPick2 = false;
 
-      Result = ImplicitConversionSequence::Worse;
-    } else {
-      // Qualifiers are disjoint.
-      return ImplicitConversionSequence::Indistinguishable;
-    }
+  if (CanPick1 != CanPick2)
+    return CanPick1 ? ImplicitConversionSequence::Better
+                    : ImplicitConversionSequence::Worse;
 
-    // If the types after this point are equivalent, we're done.
-    if (S.Context.hasSameUnqualifiedType(T1, T2))
-      break;
-  }
-
-  // Check that the winning standard conversion sequence isn't using
-  // the deprecated string literal array to pointer conversion.
-  switch (Result) {
-  case ImplicitConversionSequence::Better:
-    if (SCS1.DeprecatedStringLiteralToCharPtr)
-      Result = ImplicitConversionSequence::Indistinguishable;
-    break;
-
-  case ImplicitConversionSequence::Indistinguishable:
-    break;
-
-  case ImplicitConversionSequence::Worse:
-    if (SCS2.DeprecatedStringLiteralToCharPtr)
-      Result = ImplicitConversionSequence::Indistinguishable;
-    break;
-  }
-
-  return Result;
-}
-
 /// CompareDerivedToBaseConversions - Compares two standard conversion
 /// sequences to determine whether they can be ranked based on their
 /// various kinds of derived-to-base conversions (C++
 /// [over.ics.rank]p4b3).  As part of these checks, we also look at
 /// conversions between Objective-C interface types.
 static ImplicitConversionSequence::CompareKind
 CompareDerivedToBaseConversions(Sema &S, SourceLocation Loc,
                                 const StandardConversionSequence& SCS1,
if (!TopLevel)
       Conv |= ReferenceConversions::NestedQualification;
 
     // MS compiler ignores __unaligned qualifier for references; do the same.
     T1 = withoutUnaligned(Context, T1);
     T2 = withoutUnaligned(Context, T2);
 
     // If we find a qualifier mismatch, the types are not reference-compatible,
     // but are still be reference-related if they're similar.
     bool ObjCLifetimeConversion = false;
     if (!isQualificationConversionStep(T2, T1, /*CStyle=*/false, TopLevel,
                                        PreviousToQualsIncludeConst,
                                        ObjCLifetimeConversion))
       return (ConvertedReferent || Context.hasSimilarType(T1, T2))
                  ? Ref_Related
                  : Ref_Incompatible;
 
     // FIXME: Should we track this for any level other than the first?

clang/test/CXX/drs/dr3xx.cpp

namespace dr330 { // dr330: 7
   typedef Qinner const *Q2; // same as Q, but 'const' written outside the array type
   typedef const int *const (*R)[4];
   typedef const int *const (*S)[];
   typedef const int *(*T)[];
   void f(P p, Q q, Q2 q2, R r, S s, T t) {
     q = p; // ok
     q2 = p; // ok
     r = p; // expected-error {{incompatible}}
-    s = p; // expected-error {{incompatible}} (for now)
+    s = p;
+#if __cplusplus < 202002
+    // expected-error@-2 {{incompatible}} (fixed by p0388)
+#endif
     t = p; // expected-error {{incompatible}}
-    s = q; // expected-error {{incompatible}}
-    s = q2; // expected-error {{incompatible}}
+    s = q;
+#if __cplusplus < 202002
+    // expected-error@-2 {{incompatible}} (fixed by p0388)
+#endif
+    s = q2;
+#if __cplusplus < 202002
+    // expected-error@-2 {{incompatible}} (fixed by p0388)
+#endif
     s = t; // ok, adding const
     t = s; // expected-error {{discards qualifiers}}
     (void) const_cast<P>(q);
     (void) const_cast<P>(q2);
     (void) const_cast<Q>(p);
     (void) const_cast<Q2>(p);
     (void) const_cast<S>(p); // expected-error {{not allowed}} (for now)
     (void) const_cast<P>(s); // expected-error {{not allowed}} (for now)

clang/test/CodeGenCXX/cxx20-p0388-unbound-ary.cpp

+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++20 -emit-llvm -O2 -o - | FileCheck %s
+
+// p0388 conversions to unbounded array
+// dcl.init.list/3
+
+namespace One {
+int ga[1];
+
+// CHECK-LABEL: @_ZN3One5frob1Ev
+// CHECK-NEXT: entry:
+// CHECK-NEXT: ret [0 x i32]* bitcast ([1 x i32]* @_ZN3One2gaE to [0 x i32]*)
+auto &frob1() {
+  int(&r1)[] = ga;
+
+  return r1;
+}
+
+// CHECK-LABEL: @_ZN3One5frob2ERA1_i
+// CHECK-NEXT: entry:
+// CHECK-NEXT: %0 = bitcast [1 x i32]* %arp to [0 x i32]*
+// CHECK-NEXT: ret [0 x i32]* %0
+auto &frob2(int (&arp)[1]) {
+  int(&r2)[] = arp;
+
+  return r2;
+}
+} // namespace One

clang/test/SemaCXX/cxx20-p0388-unbound-ary.cpp

+// RUN: %clang_cc1 -std=c++20 -verify %s
+// RUN: %clang_cc1 -std=c++17 -verify %s
+
+// p0388 conversions to unbounded array
+// dcl.init.list/3
+
+namespace One {
+int ga[1];
+
+auto &frob1() {
+  int(&r1)[] = ga;
+#if __cplusplus < 202002
+  // expected-error@-2{{cannot bind to a value of unrelated type}}
+#endif
+
+  return r1;
+}
+
+auto &frob2(int (&arp)[1]) {
+  int(&r2)[] = arp;
+#if __cplusplus < 202002
+  // expected-error@-2{{cannot bind to a value of unrelated type}}
+#endif
+
+  return r2;
+}
+} // namespace One
+
+namespace Two {
+int ga[1];
+
+auto *frob1() {
+  int(*r1)[] = &ga;
+#if __cplusplus < 202002
+  // expected-error@-2{{with an rvalue of type}}
+#endif
+
+  return r1;
+}
+
+auto *frob2(int (*arp)[1]) {
+  int(*r2)[] = arp;
+#if __cplusplus < 202002
+  // expected-error@-2{{with an lvalue of type}}
+#endif
+
+  return r2;
+}
+} // namespace Two
+
+namespace Four {
+using Inc = int[2];
+using Mat = Inc[1];
+Mat *ga[2];
+
+auto *frob1() {
+  Inc(*const(*r1)[])[] = &ga;
+#if __cplusplus < 202002
+  // expected-error@-2{{with an rvalue of type}}
+#else
+  // missing a required 'const'
+  Inc(*(*r2)[])[] = &ga; // expected-error{{cannot initialize}}
+#endif
+
+  return r1;
+}
+
+auto *frob2(Mat *(*arp)[1]) {
+  Inc(*const(*r2)[])[] = arp;
+#if __cplusplus < 202002
+  // expected-error@-2{{with an lvalue of type}}
+#else
+  Inc(*(*r3)[])[] = arp; // expected-error{{cannot initialize}}
+#endif
+
+  return r2;
+}
+
+} // namespace Four