Quuxplusone added inline comments.
================ Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342 + auto w = new int(x); +} ---------------- 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. 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