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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
