[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342
+  auto w = new int(x);
+}
----------------
carlosgalvezp wrote:
> carlosgalvezp wrote:
> > Quuxplusone wrote:
> > > What about
> > > ```
> > > template<class T> T foo(int i) { return T(i); }
> > > int main() {
> > >     foo<std::vector<int>>(); // valid, OK(!)
> > >     foo<double>(); // valid, not OK
> > > }
> > > ```
> > > What about
> > > ```
> > > struct Widget { Widget(int); };
> > > using T = Widget;
> > > using U = Widget&;
> > > int i = 42;
> > > Widget t = T(i);  // valid, OK?
> > > Widget u = U(i);  // valid C++, should definitely not be OK
> > > ```
> > > https://quuxplusone.github.io/blog/2020/01/22/expression-list-in-functional-cast/
> > Good point, thanks! I've added the first one to the unit test.
> > 
> > Regarding the second check, I'm not sure if it's the scope of this check. This check does not care whether the constructor of the class is implicit or not - if you use a class type, then you are calling the constructor so it's fine. Same goes when it's a reference - in my opinion this check is not concerned with that.
> > 
> > I definitely see the problems that can arise from the example that you posted, but maybe it fits better as a separate check in the `bugprone` category? This check (`google-readability-casting`) is focused only about avoiding C-style casting, i.e. it's a readability/style/modernize matter IMO. If cpplint is not diagnosing that, I don't think this check should either.
> It seems I should be able to just add the second example as a test and clang-tidy would warn but, what should be the fixit for it? A `static_cast<U>` would lead to compiler error (which I personally would gladly take, but I don't know in general if we want clang-tidy to "fix" code leading to compiler errors"). Adding an ad-hoc message for this particular error seems out of the scope of a "readability" check. 
> 
> What do you guys think?
> It seems I should be able to just add the second example as a test and clang-tidy would warn but, what should be the fixit for it?

If I run the second example, but with old style C casts instead:

Input:
```lang=cpp
struct Widget { Widget(int); };
using T = Widget;
using U = Widget&;
int i = 42;
Widget t = (T)(i);
Widget u = (U)(i);
```

Output after fixits:
```lang=cpp
struct Widget { Widget(int); };
using T = Widget;
using U = Widget&;
int i = 42;
Widget t = T(i);
Widget u = (U)(i);
```

I guess the fix `Widget t = T(i);` is OK as it is covered by this exception:
>You may use cast formats like `T(x)` only when `T` is a class type.

For the `Widget u = (U)(i);` line, clang-tidy has warned about it but not offered a fix.


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

https://reviews.llvm.org/D114427