carlosgalvezp added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:99-106
+ CharSourceRange ReplaceRange;
+ if (isa<CStyleCastExpr>(CastExpr))
+ ReplaceRange = CharSourceRange::getCharRange(
+ CastExpr->getLParenLoc(),
+ CastExpr->getSubExprAsWritten()->getBeginLoc());
+ else if (isa<CXXFunctionalCastExpr>(CastExpr))
+ ReplaceRange = CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
----------------
salman-javed-nz wrote:
> The majority of `checkExpr()`'s contents are common to both types,
> `CStyleCastExpr` and `CXXFunctionalCastExpr`.
> Only the `ReplaceRange = CharSourceRange::getCharRange...` and the
> `DestTypeString = Lexer::getSourceText...` parts change depending on the Expr
> type.
>
> What about breaking those two assignments out into their own functions,
> rather than templating the entire `checkExpr()` function?
>
> For example, (note: untested code)
>
> ```lang=cpp
> clang::CharSourceRange GetReplaceRange(const CStyleCastExpr *CastExpr) {
> return CharSourceRange::getCharRange(
> CastExpr->getLParenLoc(),
> CastExpr->getSubExprAsWritten()->getBeginLoc());
> }
>
> clang::CharSourceRange GetReplaceRange(const CXXFunctionalCastExpr *CastExpr)
> {
> return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
> CastExpr->getLParenLoc());
> }
>
> ...
>
> CharSourceRange ReplaceRange =
> isa<CStyleCastExpr>(CastExpr)
> ? GetReplaceRange(dyn_cast<const CStyleCastExpr>(CastExpr))
> : GetReplaceRange(dyn_cast<const CXXFunctionalCastExpr>(CastExpr));
> ```
>
> Would something like that work?
Thanks for the suggestion, much cleaner! I've made `CastExpr` become a
`ExplicitCastExpr` instead (which is common base to both cast classes) to be
able to handle only one object.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:335
+ const char *str = "foo";
+ auto s = S(str);
+}
----------------
salman-javed-nz wrote:
> Is a test to check `new int(x)` worth including? I see that the cpplint guys
> explicitly filter it out of their results.
Sure, even though I think technically it's not a cast. At least it's not shown
as such in the AST.
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
