carlosgalvezp added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:170 + : getDestTypeString(SM, getLangOpts(), + dyn_cast<const CXXFunctionalCastExpr>(CastExpr)); ---------------- Quuxplusone wrote: > 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); > ``` Updated to move the conditional into the function. I cannot avoid the casting though, because there is no common base class of `CXXFunctionalCastExpr` and `CStyleCastExpr` that has the methods `getLParenLoc` and so on, so that's why I need the type explicitly instead of invoking those methods from the base class. The original solution used templates instead of overloading for this, but @salman-javed-nz suggested overloading instead. I think that's a bit easier to read IMO, with small functions having a single responsibility. Let me know if you like the update :) ================ 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); ---------------- Quuxplusone wrote: > 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. Yeah I'd prefer that too, but I wasn't sure how to set expectations on the same line for different inputs. 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
