[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Carlos Galvez via Phabricator via cfe-commits]

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