This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Improve const_cast conformance to N4261
Needs ReviewPublic

Authored by Mordante on Sep 12 2020, 10:53 AM.

Details

Reviewers
rjmccall
rsmith
Summary

Allows const_casts of similar non-record types.

Diff Detail

Event Timeline

Mordante requested review of this revision.Sep 12 2020, 10:53 AM
Mordante created this revision.
rsmith requested changes to this revision.Sep 13 2020, 4:36 PM
rsmith added inline comments.
clang/lib/Sema/SemaCast.cpp
1781–1782

This is the bullet that causes class pvalues and non-class prvalues to be treated differently by const_cast to an rvalue reference type, and it still exists unchanged in the latest C++ draft. As far as I can tell, the change here is not in line with the wording for N4261 / CWG issue 330, which Clang does implement.

(Now, I think the change you're suggesting here makes a lot of sense, but we should get agreement in WG21 to fix the language rules first.)

1797–1801

In (for example) const_cast<int&&>(0);, T1 is int, and T2 is "rvalue reference to int". T1 and T2 are not similar types, because they have different types U (for T1, U is int; for T2, U is "rvalue reference to int"). So this doesn't apply.

This revision now requires changes to proceed.Sep 13 2020, 4:36 PM

For what it's worth, the most recent thoughts from CWG on this question are from 2015 (prior to guaranteed copy elision, which probably changes the answer): http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1965. That direction would permit const_cast to rvalue reference type to materialize a temporary for arrays and classes but not scalar types (which seems just as arbitrary as the current set of rules to me).

Mordante marked 2 inline comments as done.Oct 11 2020, 5:13 AM

Thanks for the additional information. I agree that the current situation seems a bit arbitrary.
I did some further research to see what GCC allows https://godbolt.org/. I've updated the patch to match that behavior.

Mordante updated this revision to Diff 297456.Oct 11 2020, 5:16 AM

Match the behavior of const_cast to match GCC's behavior. N4261 was proposed as solution to DR330. Since GCC allows the new const_cast behavior retroactively the patch also matches that behavior and no longer restricts the new usage to C++17.

rsmith added inline comments.Oct 14 2020, 6:22 PM
clang/lib/Sema/SemaCast.cpp
1806–1808

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.

Mordante marked an inline comment as done.Oct 17 2020, 10:39 AM
Mordante added inline comments.
clang/lib/Sema/SemaCast.cpp
1806–1808

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.

Mordante updated this revision to Diff 298838.Oct 17 2020, 10:42 AM
Mordante marked an inline comment as done.

Removes the support for the pointer types.

I adjusted the unit test for void f(), but I think it would be better remove this test now pointers are no longer allowed. Do you agree?

Friendly ping.