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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG5bbe50148f3b: [clang-tidy] Warn on functional C-style casts (authored by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

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

carlosgalvezp added a comment. Thanks a lot for the review! I've fixed the last comment by @salman-javed-nz . Since that was the last standing comment I'll push :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114427/new/ https://reviews.llvm.org/D114427 __

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

carlosgalvezp updated this revision to Diff 390580. carlosgalvezp added a comment. Fixed doc. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114427/new/ https://reviews.llvm.org/D114427 Files: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp clang-tools-extra/docs/Relea

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

Quuxplusone added a comment. LGTM! Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:318 // FIXME: This should be a static_cast. // C HECK-FIXES: auto s5 = static_cast(cr); auto s6 = (S)cr; salman-javed-nz wrote: >

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

salman-javed-nz added a comment. Nothing else that comes to mind that I want to see. @Quuxplusone are you OK with the latest round of diffs? Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:318 // FIXME: This should be a static_cast.

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

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:63-73 +static clang::CharSourceRange getReplaceRange(const ExplicitCastExpr *Expr) { + if (const auto *CastExpr = dyn_cast(Expr)) { +return CharSourceRange::getCh

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

carlosgalvezp updated this revision to Diff 390394. carlosgalvezp added a comment. Move logic into single functions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114427/new/ https://reviews.llvm.org/D114427 Files: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp clang

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

Quuxplusone added inline comments. Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:63-79 +static clang::CharSourceRange getReplaceRange(const CStyleCastExpr *CastExpr) { + return CharSourceRange::getCharRange( + CastExpr->getLParenLoc(), CastExpr-

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

Quuxplusone added inline comments. Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:101-110 +static StringRef getDestTypeString(const SourceManager &SM, + const LangOptions &LangOpts, +

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

carlosgalvezp added a comment. In D114427#3158405 , @Quuxplusone wrote: > Marking "accepted" for the record; but my checkmark means merely "I'm not > intending to block this," not "I claim the authority to say you //should// > land this." :) Thanks! I

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:170 + : getDestTypeString(SM, getLangOpts(), + dyn_cast(CastExpr)); Quuxplusone wrote: > IMO, this repeated cond

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

carlosgalvezp updated this revision to Diff 390380. carlosgalvezp marked 6 inline comments as done. carlosgalvezp added a comment. Moved conditionals inside function body. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114427/new/ https://reviews.llvm.org/D114427 Files: clang-tools-ex

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

Quuxplusone accepted this revision. Quuxplusone added a comment. Marking "accepted" for the record; but my checkmark means merely "I'm not intending to block this," not "I claim the authority to say you //should// land this." :) Comment at: clang-tools-extra/clang-tidy/google

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342 + auto w = new int(x); +} Quuxplusone wrote: > salman-javed-nz wrote: > > carlosgalvezp wrote: > > > carlosgalvezp wrote: > > > > Quux

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

carlosgalvezp updated this revision to Diff 390352. carlosgalvezp added a comment. Added Widget test case (reusing the S2 struct instead of adding a new struct Widget) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114427/new/ https://reviews.llvm.org/D114427 Files: clang-tools-extra

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

Quuxplusone added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342 + auto w = new int(x); +} salman-javed-nz wrote: > carlosgalvezp wrote: > > carlosgalvezp wrote: > > > Quuxplusone wrote: > > > > What a

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

salman-javed-nz added a comment. I think the primary goal is satisfied - in all cases the cast is identified and a warning is generated. For the `Widget&` case, a warning is generated but no fixit is offered, but that isn't any worse than the existing C-style cast fixits. It does sound like a c

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

salman-javed-nz added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342 + auto w = new int(x); +} carlosgalvezp wrote: > carlosgalvezp wrote: > > Quuxplusone wrote: > > > What about > > > ``` > > > templa

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

carlosgalvezp added a comment. Personally I think it's best to merge this as is, allowing people to e.g. replace cpplint with clang-tidy (one static analyzer to rule them all!), and add more functionality in a separate, focused patch. Looking forward to hearing your thoughts :) CHANGES SINCE

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342 + auto w = new int(x); +} carlosgalvezp wrote: > Quuxplusone wrote: > > What about > > ``` > > template T foo(int i) { return T(i); }

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342 + auto w = new int(x); +} Quuxplusone wrote: > What about > ``` > template T foo(int i) { return T(i); } > int main() { > foo>();

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

carlosgalvezp updated this revision to Diff 390191. carlosgalvezp added a comment. Add extra test for template functions that use the functional cast. It should only warn when T is not a class type. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114427/new/ https://reviews.llvm.org/D114

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

Quuxplusone added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342 + auto w = new int(x); +} What about ``` template T foo(int i) { return T(i); } int main() { foo>(); // valid, OK(!) foo(); // v

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

salman-javed-nz accepted this revision. salman-javed-nz added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:335 + const char *str = "foo"; + auto s = S(str); +}

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:122 + ? getReplaceRange(dyn_cast(CastExpr)) + : getReplaceRange(dyn_cast(CastExpr)); One problem I see here is in the future someone

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:99-106 + CharSourceRange ReplaceRange; + if (isa(CastExpr)) +ReplaceRange = CharSourceRange::getCharRange( +CastExpr->getLParenLoc(), +CastExpr->ge

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

carlosgalvezp updated this revision to Diff 390165. carlosgalvezp marked 5 inline comments as done. carlosgalvezp added a comment. Addressed comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114427/new/ https://reviews.llvm.org/D114427 Files: clang-tools-extra/clang-tidy/google

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

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:100-104 + if (isa(CastExpr)) +ReplaceRange = CharSourceRange::getCharRange( +CastExpr->getLParenLoc(), +CastExpr->getSubExprAsWritten()->getBegin

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

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(CastExpr)) +Repl

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

carlosgalvezp updated this revision to Diff 389159. carlosgalvezp added a comment. Fix numbering in variables. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114427/new/ https://reviews.llvm.org/D114427 Files: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp clang-tools

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

carlosgalvezp updated this revision to Diff 389157. carlosgalvezp added a comment. Fix numbering of variables. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114427/new/ https://reviews.llvm.org/D114427 Files: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp clang-tools

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

carlosgalvezp created this revision. Herald added a subscriber: xazax.hun. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. The google-readability-casting check is meant to be on par with cpplint's readability/casti