[PATCH] D87565: [Sema] Improve const_cast conformance to N4261

[Mark de Wever via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/Sema/SemaCast.cpp:1817-1819
+      NeedToMaterializeTemporary =
+          SrcType->isRecordType() || SrcType->isArrayType() ||
+          SrcType->isFunctionPointerType() || SrcType->isMemberPointerType();
----------------
rsmith wrote:
> It looks like GCC allowing a cast from `T*` to `T*&&` is just a bug in their implementation. Consider:
> 
> ```
> using T = int*;
> T &&r1 = const_cast<T&&>(T{}); // GCC accepts
> T &&r2 = const_cast<T&&>(T()); // GCC rejects
> ```
> 
> ... and the same behavior seems to show up for all scalar types: they permit `const_cast` from `T{}` but not from `T()` when `T` is `int`, `int*`, .... This doesn't seem like good behavior to follow. I think we should either implement the current direction of 1965 (that is, only allow classes and arrays here) or leave the current behavior (following the standard as-is) alone.
> It looks like GCC allowing a cast from `T*` to `T*&&` is just a bug in their implementation. Consider:
> 
> ```
> using T = int*;
> T &&r1 = const_cast<T&&>(T{}); // GCC accepts
> T &&r2 = const_cast<T&&>(T()); // GCC rejects
> ```
> 
> ... and the same behavior seems to show up for all scalar types: they permit `const_cast` from `T{}` but not from `T()` when `T` is `int`, `int*`, ....

Interesting I hadn't test with `T()` only with `T{}`

> This doesn't seem like good behavior to follow.

Agreed.

> I think we should either implement the current direction of 1965 (that is, only allow classes and arrays here)

I'll then limit the patch to array types (next to the already allowed record types.

> or leave the current behavior (following the standard as-is) alone.

Now I'm somewhat confused, because I thought arrays are allowed in the standard http://eel.is/c++draft/expr.const.cast#3 as introduced by N4261.



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

https://reviews.llvm.org/D87565