const_cast and p0388 (array boundloss becomes const_cast)

P0388 http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0388r4.html Makes conversions between bounded-array and unbounded-array look like qualification conversions. This means you can use const-cast to add array bounds (and implicitly lose bounds)

But, ConstCast has a cast-kind of CK_NoOp:
AST/ExprCXX.h:
   CXXConstCastExpr(QualType ty, ExprValueKind VK, Expr *op,
                    TypeSourceInfo *writtenTy, SourceLocation l,
                    SourceLocation RParenLoc, SourceRange AngleBrackets)
       : CXXNamedCastExpr(CXXConstCastExprClass, ty, VK, CK_NoOp, op, 0,
                          /*HasFPFeatures*/ false, writtenTy, l, RParenLoc,
                          AngleBrackets) {}

That will no longer be true -- adding or removing bounds changes the type more than CK_NoOp permits. This results in code emission assert failure when things like [4 x i32]* meets [0 x i32]*.

so, should such constcasts generate non-ConstCast AST?, or should the ConstCast node sometimes not be CK_NoOp? The latter seems better to me, but perhaps there's a different opinion?

nathan

P0388
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0388r4.html
Makes conversions between bounded-array and unbounded-array look like
qualification conversions. This means you can use const-cast to add
array bounds (and implicitly lose bounds)

I think we changed the const_cast rules to disallow that. Here:

Modify 7.6.1.10 [expr.const.cast] ¶3:

“For two similar types T1 and T2, a prvalue of type T1 may be explicitly converted to the type T2 using a const_cast if, considering the cv-decompositions ([conv.qual]) of both types, all Pi1 are the same as Pi2.”

A change in array bound would result in the Pi’s differing between T1 and T2, so a const_cast can’t do that.

But, ConstCast has a cast-kind of CK_NoOp:
AST/ExprCXX.h:
CXXConstCastExpr(QualType ty, ExprValueKind VK, Expr *op,
TypeSourceInfo *writtenTy, SourceLocation l,
SourceLocation RParenLoc, SourceRange AngleBrackets)
: CXXNamedCastExpr(CXXConstCastExprClass, ty, VK, CK_NoOp, op, 0,
/HasFPFeatures/ false, writtenTy, l, RParenLoc,
AngleBrackets) {}

That will no longer be true – adding or removing bounds changes the
type more than CK_NoOp permits. This results in code emission assert
failure when things like [4 x i32]* meets [0 x i32]*.

so, should such constcasts generate non-ConstCast AST?, or should the
ConstCast node sometimes not be CK_NoOp? The latter seems better to me,
but perhaps there’s a different opinion?

Regardless of the above observation about const_cast, I think we do need to think about the cast kind for array bound conversions, because implicit conversions can now remove an array bound, and static_cast can now add one. Seems like the choices are CK_NoOp, CK_BitCast, or something new. I don’t like using CK_BitCast for this, since that usually implies something more violent is happening (and would cause problems for constant evaluation, which generally allows pointer bitcasts).

CK_NoOp is really a misnomer, and should probably be renamed to something that indicates what it actually models: a change in qualifiers. I think that changing array bounds is notionally the same kind of type change as changing type qualifiers or ‘noexcept’ on a function type (even though in IR it will result in a type change until the work to remove pointee types is completed), so my inclination would be to call that a CK_NoOp too, unless we have a reason not to – and that’s probably also easier because it means we don’t need to work out how to decompose an arbitrary qualification conversion into a change of cv-qualifiers and a separate change of array bounds. I think it should be relatively straightforward to get CodeGen to insert bitcasts for CK_NoOp casts where necessary.

On the subject of constant evaluation, do we have clear rules on whether things like this work:

constexpr int a[3][4] = {};
constexpr int (*p)[] = a;
constexpr int (q)[12] = static_cast<int ()[12]>(p);
constexpr int r = (*q)[5];

? I think that’s probably valid up until the final line, which we should reject for out-of-bounds array indexing because q points to an array of 4 ints. (Though it might be nicer if we could reject the incorrect array bound in the static_cast.) I think that’s the behavior we’ll get by modeling this as a CK_NoOp.

    P0388
    http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0388r4.html
    <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0388r4.html>
    Makes conversions between bounded-array and unbounded-array look like
    qualification conversions. This means you can use const-cast to add
    array bounds (and implicitly lose bounds)

I think we changed the const_cast rules to disallow that. Here:

Modify 7.6.1.10 [expr.const.cast] ¶3:

"For two similar types T1 and T2, a prvalue of type T1 may be explicitly converted to the type T2 using a const_cast <ins>if, considering the cv-decompositions ([conv.qual]) of both types, all Pi1 are the same as Pi2</ins>."

A change in array bound would result in the Pi's differing between T1 and T2, so a const_cast can't do that.

Ah, I'd misunderstood that change, /and/ paid too much attention to 'modelled like a cv qualifier'.

Regardless of the above observation about const_cast, I think we do need to think about the cast kind for array bound conversions, because implicit conversions can now remove an array bound, and static_cast can now add one. Seems like the choices are CK_NoOp, CK_BitCast, or something new. I don't like using CK_BitCast for this, since that usually implies something more violent is happening (and would cause problems for constant evaluation, which generally allows pointer bitcasts).

Agreed. (CK_LValueBitCast also looked applicable)

CK_NoOp is really a misnomer, and should probably be renamed to something that indicates what it actually models: a change in qualifiers. I think that changing array bounds is notionally the same kind of type change as changing type qualifiers or 'noexcept' on a function type (even though in IR it will result in a type change until the work to remove pointee types is completed), so my inclination would be to call that a CK_NoOp too, unless we have a reason not to -- and that's probably also easier because it means we don't need to work out how to decompose an arbitrary qualification conversion into a change of cv-qualifiers and a separate change of array bounds. I think it should be relatively straightforward to get CodeGen to insert bitcasts for CK_NoOp casts where necessary.

The fly in that ointment is, IIUC, that the array bounds changes are visible in IR, but the cv qualifier and noexcept changes are not? So we have to emit some type conversion IR to avoid mismatches there -- that is how I ran into this problem after all. I will go poke some more. Perhaps the cast emission for CK_NoOp (line 4674 of CGExpr.cpp)
     return EmitLValue(E->getSubExpr());
needs to do more, sometimes.

P0388
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0388r4.html
<http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0388r4.html>
Makes conversions between bounded-array and unbounded-array look like
qualification conversions. This means you can use const-cast to add
array bounds (and implicitly lose bounds)

I think we changed the const_cast rules to disallow that. Here:

Modify 7.6.1.10 [expr.const.cast] ¶3:

“For two similar types T1 and T2, a prvalue of type T1 may be explicitly
converted to the type T2 using a const_cast if, considering the
cv-decompositions ([conv.qual]) of both types, all Pi1 are the same as
Pi2
.”

A change in array bound would result in the Pi’s differing between T1
and T2, so a const_cast can’t do that.

Ah, I’d misunderstood that change, /and/ paid too much attention to
‘modelled like a cv qualifier’.

Regardless of the above observation about const_cast, I think we do need
to think about the cast kind for array bound conversions, because
implicit conversions can now remove an array bound, and static_cast can
now add one. Seems like the choices are CK_NoOp, CK_BitCast, or
something new. I don’t like using CK_BitCast for this, since that
usually implies something more violent is happening (and would cause
problems for constant evaluation, which generally allows pointer bitcasts).

Agreed. (CK_LValueBitCast also looked applicable)

CK_NoOp is really a misnomer, and should probably be renamed to
something that indicates what it actually models: a change in
qualifiers. I think that changing array bounds is notionally the same
kind of type change as changing type qualifiers or ‘noexcept’ on a
function type (even though in IR it will result in a type change until
the work to remove pointee types is completed), so my inclination would
be to call that a CK_NoOp too, unless we have a reason not to – and
that’s probably also easier because it means we don’t need to work out
how to decompose an arbitrary qualification conversion into a change of
cv-qualifiers and a separate change of array bounds. I think it should
be relatively straightforward to get CodeGen to insert bitcasts for
CK_NoOp casts where necessary.

The fly in that ointment is, IIUC, that the array bounds changes are
visible in IR, but the cv qualifier and noexcept changes are not? So we
have to emit some type conversion IR to avoid mismatches there – that
is how I ran into this problem after all. I will go poke some more.
Perhaps the cast emission for CK_NoOp (line 4674 of CGExpr.cpp)
return EmitLValue(E->getSubExpr());
needs to do more, sometimes.

Yes, I think we should create an IR-level bitcast where necessary here. At least one path through IR generation already does (EmitPointerWithAlignment), but it looks like we mostly assume that CK_NoOp is, well, a no-op for IR generation. (There’s ongoing work to replace the IR-level “pointer to T” type family with a single “pointer” type, which would remove the need for a bitcast, but I think that’s probably still months or years from being complete and I wouldn’t imagine we want to wait for that.)