[PATCH] D76572: Replace `T(x)` with `reinterpret_cast<T>(x)` everywhere it means reinterpret_cast. No functional change

Mon Mar 23 13:06:37 PDT 2020

Quuxplusone added a comment.

In D76572#1937558 <https://reviews.llvm.org/D76572#1937558>, @rsmith wrote:

> In D76572#1935791 <https://reviews.llvm.org/D76572#1935791>, @lebedev.ri wrote:
>
> > Passing-by remark:
> >
> > > I wrote a Clang warning [not pictured] to diagnose any use of `T(x)` which was not equivalent to `static_cast<T>(x)`.
> >
> > I'm not sure whether or not this will pass the bar for a clang diagnostic
>
>
> I'd like to try it out on a larger codebase, but it sounds at least potentially good to me. There's a simple syntactic workaround (use `(T)x` instead of `T(x)`), and there's a high likelihood that the code doesn't mean what the programmer intended.
>
> Does the warning catch the cases where the code is equivalent to `static_cast<T>(x)` except that it ignores access? That seems like a really good thing to warn on regardless of whether we warn on the general case.

Yes, I verified with some simple test cases that it catches the equivalents of (1) reinterpret_cast, (2) const_cast, (3) const_cast plus static_cast, (4) upcast to private base, and (5) downcast to private base.
However, the thing I wrote is 100% **not** ready for prime time. It basically just inserts a call to `TryStaticCast` from inside `CheckCXXCStyleCast`. Unfortunately `TryStaticCast` is one of these functions that Clang is full of, where it both "checks if X is possible" and also "does X" and also "prints error messages to stdout," all from within the same function. So my patch ends up being morally equivalent to just treating `T(x)` as `static_cast<T>(x)`; it emits some errors that shouldn't be there if it's supposed to just warn, and I have no idea how to shut up those errors except by completely refactoring `TryXXXXCast`. (E.g., it could take an extra parameter `Mode` that indicated whether to actually do the conversion, or just report whether it was possible, or emit notes about why it was impossible.) I wrestled unsuccessfully with similar issues when I did `-Wreturn-std-move`.

I 100% want to see someone like you tackle this issue, but take my word for it, you **don't** want to start with the code I wrote the other day. :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76572