carlosgalvezp added inline comments.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342
+ auto w = new int(x);
+}
----------------
Quuxplusone wrote:
> salman-javed-nz wrote:
> > 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.
> > What would be the right fixit for that anyway?
> > `Widget u = U(i); --> Widget u = static_cast<T>(i);` ?
>
> No, this is a reinterpret_cast, so it would be
> ```
> Widget u = reinterpret_cast<U>(i);
> ```
> at least in C++. I don't know about C, but I imagine the problem doesn't come
> up.
>
> (If the programmer looks at this line and says "oh geez, that's wrong," well,
> he'll either fix it or file a task to revisit weird reinterpret_casts in the
> codebase. If the programmer thinks the cast is //correct//, then personally
> I'd hope he rewrites it as `Widget u = *reinterpret_cast<Widget*>(&i);` for
> clarity, but that's not a clang-tidy issue.)
>
> Relevant: "fixits versus suppression mechanisms"
> https://quuxplusone.github.io/blog/2020/09/02/wparentheses/
> `reinterpret_cast` is a suppression mechanism; I infer that you're casting
> about for a fixit, which won't exist in this case.
Added test case, currently it provides a generic comment.
Thanks a lot for the explanation, this was eye-opening to me. I only thought of
reinterpret casts when using pointers, but of course references are kind of the
same thing :)
Let me know if you are happy with the patch!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114427/new/
https://reviews.llvm.org/D114427
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
