Quuxplusone accepted this revision.
Quuxplusone added a comment.
Marking "accepted" for the record; but my checkmark means merely "I'm not
intending to block this," not "I claim the authority to say you //should// land
this." :)
================
Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:170
+ : getDestTypeString(SM, getLangOpts(),
+ dyn_cast<const CXXFunctionalCastExpr>(CastExpr));
----------------
IMO, this repeated conditional (compare lines 119–122) should be factored out
into the //body// of the helper function `getDestTypeString` (rather than being
repeated at every call-site), and `getDestTypeString` should take a `const
ExplicitCastExpr *` instead of having two overloads. (Notice that you never
//use// the overloading for anything: everywhere you call into the overload
set, you do so with a non-dependent `dyn_cast` wrapped in a `?:`, indicating
that you don't really want overloading at all.)
```
StringRef DestTypeString = getDestTypeString(SM, getLangOpts(), CastExpr);
```
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:358-360
+ // Functional casts in template functions
+ functional_cast_template_used_by_class<S2>(x);
+ functional_cast_template_used_by_int<int>(x);
----------------
FWIW, I'd prefer to instantiate the same function template in both cases
(because that's the interesting case for practical purposes — a template that's
only instantiated once doesn't pose a problem for the programmer). But I get
that you're doing this because it's easier to express the expected output.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:369
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C-style casts are discouraged;
use static_cast/const_cast/reinterpret_cast
+}
----------------
It has also just now occurred to me that it might be interesting to see what
happens with this warning in a SFINAE context, e.g.
```
template<class T> auto is_castable(int i) -> decltype(T(i), void(), true) {
return true; }
```
but I suppose none of the things we could do there would be remotely sensible,
so you'd just be testing basically that clang-tidy doesn't crash in that case.
Anyway, no action needed.
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
