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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Thanks for the patch.

Just a little bit of feedback but overall I'm happy with the approach taken.



================
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(),
----------------
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?


================
Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:151-162
+  if (isa<CStyleCastExpr>(CastExpr))
+    DestTypeString =
+        Lexer::getSourceText(CharSourceRange::getTokenRange(
+                                 CastExpr->getLParenLoc().getLocWithOffset(1),
+                                 CastExpr->getRParenLoc().getLocWithOffset(-1)),
+                             SM, getLangOpts());
+  else if (isa<CXXFunctionalCastExpr>(CastExpr))
----------------
See comment above.


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:139-140
 
+- Updated `google-readability-casting` to diagnose and fix functional casts, to achieve feature
+  parity with the corresponding `cpplint.py` check.
 
----------------
Single back-ticks are used to do linking. Double back-ticks is probably what you're after.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:335
+  const char *str = "foo";
+  auto s = S(str);
+}
----------------
Is a test to check `new int(x)` worth including? I see that the cpplint guys explicitly filter it out of their results.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114427/new/

https://reviews.llvm.org/D114427