RE: [PATCH] D16310: new clang-tidy checker misc-long-cast

aron.ball...@gmail.com] Skickat: den 10 februari 2016 14:12 Till: Daniel Marjamäki Kopia: reviews+d16310+public+4a30d2d495388...@reviews.llvm.org; ale...@google.com; legal...@xmission.com; eugene.zele...@gmail.com; cfe-commits@lists.llvm.org Ämne: Re: [PATCH] D16310: new clang-tidy checker misc-long-cast On

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

In article , Daniel Marjamäki writes: > > You can get overflow with / and %. Consider: > > > > int i = INT_MIN / -1; // similar for % > > you are right, I did not think of that. > > imho overflow

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki added inline comments. Comment at: clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp:21-23 @@ +20,5 @@ +void MisplacedWideningCastCheck::registerMatchers(MatchFinder *Finder) { + auto Calc = expr(anyOf(binaryOperator(anyOf( +

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

e.zele...@gmail.com; > cfe-commits@lists.llvm.org > Ämne: Re: [PATCH] D16310: new clang-tidy checker misc-long-cast > > LegalizeAdulthood added inline comments. > > > Comment at: > clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp:21-23 @@ +20,5 @@ +void MisplacedWideningCastCheck::registerMatchers(MatchFinder *Finder) { + auto Calc = expr(anyOf(binaryOperator(anyOf( +

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. Looks good after fixing capitalization in comments. Thank you! Comment at: clang-tidy/misc/MisplacedWideningCastCheck.cpp:32 @@ +31,3 @@ +

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki added inline comments. Comment at: docs/clang-tidy/checks/misc-misplaced-widening-cast.rst:2 @@ +1,3 @@ +.. title:: clang-tidy - misc-misplaced-widening-cast + +misc-misplaced-widening-cast For information, it seem I had to use

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki added inline comments. Comment at: clang-tidy/misc/MisplacedWideningCastCheck.cpp:31 @@ +30,3 @@ + + Finder->addMatcher(returnStmt(has(Cast)), this); + Finder->addMatcher(varDecl(has(Cast)), this); alexfh wrote: > FYI, these matchers can be

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki updated this revision to Diff 47182. danielmarjamaki marked 9 inline comments as done. danielmarjamaki added a comment. Fixed review comments http://reviews.llvm.org/D16310 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki added inline comments. Comment at: clang-tidy/misc/MisplacedWideningCastCheck.cpp:79 @@ +78,3 @@ + +void MisplacedWideningCastCheck::check(const MatchFinder::MatchResult ) { + const auto *Cast = Result.Nodes.getNodeAs("Cast"); I looked in

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki updated this revision to Diff 47181. danielmarjamaki added a comment. Fixes according to review comments. http://reviews.llvm.org/D16310 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/MisplacedWideningCastCheck.cpp

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki marked an inline comment as done. danielmarjamaki added a comment. ping? http://reviews.llvm.org/D16310 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

alexfh added inline comments. Comment at: clang-tidy/misc/MisplacedWideningCastCheck.cpp:20 @@ +19,3 @@ +void MisplacedWideningCastCheck::registerMatchers(MatchFinder *Finder) { + auto Calc = stmt(anyOf(binaryOperator(anyOf( + hasOperatorName("+"),

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki marked an inline comment as done. Comment at: clang-tidy/misc/MisplacedWideningCastCheck.cpp:33 @@ +32,3 @@ + Finder->addMatcher(varDecl(has(Cast)), this); + Finder->addMatcher(binaryOperator(hasOperatorName("="), hasRHS(Cast)), this); +} I have

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki updated this revision to Diff 46235. danielmarjamaki added a comment. Refactoring the AST matching http://reviews.llvm.org/D16310 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/MisplacedWideningCastCheck.cpp

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki added a comment. In http://reviews.llvm.org/D16310#337563, @LegalizeAdulthood wrote: > In http://reviews.llvm.org/D16310#335844, @danielmarjamaki wrote: > > > There were some new interesting warnings and imho they were TP. > > > TP? Sorry.. True Positive

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

LegalizeAdulthood added a comment. In http://reviews.llvm.org/D16310#335844, @danielmarjamaki wrote: > There were some new interesting warnings and imho they were TP. TP? http://reviews.llvm.org/D16310 ___ cfe-commits mailing list

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki added a comment. For information I have now tested latest patch. Statistics: projects: 1804 files:85976 warnings: 100 There were some new interesting warnings and imho they were TP. http://reviews.llvm.org/D16310 ___

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki marked 5 inline comments as done. danielmarjamaki added a comment. http://reviews.llvm.org/D16310 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki marked 3 inline comments as done. Comment at: docs/clang-tidy/checks/misc-long-cast.rst:11 @@ +10,3 @@ + +Example code:: + alexfh wrote: > LegalizeAdulthood wrote: > > Please add an example for another type other than `long`, such as casting a >

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki added a comment. In http://reviews.llvm.org/D16310#331538, @LegalizeAdulthood wrote: > If you state what the check does, then > > In http://reviews.llvm.org/D16310#331054, @danielmarjamaki wrote: > > > In http://reviews.llvm.org/D16310#330367, @LegalizeAdulthood wrote: > > > > >

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

LegalizeAdulthood added a comment. I find this an interesting discussion. I do not mean to imply that my remarks constitute any sort of demand that this check produce my suggestion for a fixit. In http://reviews.llvm.org/D16310#333416, @danielmarjamaki wrote: > I expect that this warning will

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki added a comment. In http://reviews.llvm.org/D16310#331538, @LegalizeAdulthood wrote: > If you state what the check does, then > > In http://reviews.llvm.org/D16310#331054, @danielmarjamaki wrote: > > > In http://reviews.llvm.org/D16310#330367, @LegalizeAdulthood wrote: > > > > >

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki marked an inline comment as done. Comment at: clang-tidy/misc/LongCastCheck.cpp:21 @@ +20,3 @@ + Finder->addMatcher( + returnStmt( + has(cStyleCastExpr(has(binaryOperator(anyOf(hasOperatorName("+"), alexfh wrote: > Any reason to

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki marked an inline comment as done. Comment at: clang-tidy/misc/LongCastCheck.cpp:43 @@ +42,3 @@ + +static unsigned getMaxCalculationWidth(ASTContext , const Expr *E) { + E = E->IgnoreParenImpCasts(); LegalizeAdulthood wrote: > Prefer anonymous

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

alexfh added inline comments. Comment at: clang-tidy/misc/LongCastCheck.cpp:21 @@ +20,3 @@ + Finder->addMatcher( + returnStmt( + has(cStyleCastExpr(has(binaryOperator(anyOf(hasOperatorName("+"), Any reason to limit this to returnStmt, varDecl and

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

alexfh added inline comments. Comment at: clang-tidy/misc/LongCastCheck.cpp:21 @@ +20,3 @@ + Finder->addMatcher( + returnStmt( + has(cStyleCastExpr(has(binaryOperator(anyOf(hasOperatorName("+"), danielmarjamaki wrote: > alexfh wrote: > > Any reason

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

LegalizeAdulthood added a comment. In http://reviews.llvm.org/D16310#332200, @danielmarjamaki wrote: > Why is there a cast in the first place? It is unlikely that the programmer > added a useless cast for no reason. If this has universally been your experience on a code base, then I say you

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

LegalizeAdulthood added inline comments. Comment at: clang-tidy/misc/LongCastCheck.cpp:97 @@ +96,3 @@ + + if (!CastType->isIntegerType() || !SubType->isIntegerType()) +return; danielmarjamaki wrote: > LegalizeAdulthood wrote: > > Why don't you check for

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

LegalizeAdulthood added a comment. If you state what the check does, then In http://reviews.llvm.org/D16310#331054, @danielmarjamaki wrote: > In http://reviews.llvm.org/D16310#330367, @LegalizeAdulthood wrote: > > > Why not supply a fixit that removes the cast? > > > I am skeptic. There are

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

LegalizeAdulthood added inline comments. Comment at: clang-tidy/misc/LongCastCheck.cpp:43 @@ +42,3 @@ + +static unsigned getMaxCalculationWidth(ASTContext , const Expr *E) { + E = E->IgnoreParenImpCasts(); Prefer anonymous namespace over `static` to scope

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki removed rL LLVM as the repository for this revision. danielmarjamaki updated this revision to Diff 45378. danielmarjamaki added a comment. Fixed review comment; s/checker/check/ http://reviews.llvm.org/D16310 Files: clang-tidy/misc/CMakeLists.txt

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki added a comment. In http://reviews.llvm.org/D16310#330367, @LegalizeAdulthood wrote: > Why not supply a fixit that removes the cast? I am skeptic. There are different valid fixes. Example code: l = (long)(a*1000); Fix1: l = ((long)a * 1000); Fix2: l = (a *

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

danielmarjamaki added a comment. In http://reviews.llvm.org/D16310#330312, @Eugene.Zelenko wrote: > Clang-tidy has 6 cast related checks. May be this is good time to introduce > dedicated category for them? I am not against this. I don't know which ones you are thinking about.. but if a

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

Eugene.Zelenko added a comment. Clang-tidy has 6 cast related checks. May be this is good time to introduce dedicated category for them? Repository: rL LLVM http://reviews.llvm.org/D16310 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

Eugene.Zelenko added a subscriber: Eugene.Zelenko. Comment at: docs/clang-tidy/checks/misc-long-cast.rst:6 @@ +5,3 @@ + +This checker will warn when there is a explicit redundant cast of a calculation +result to a bigger type. If the intention of the cast is to avoid loss of

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

LegalizeAdulthood added a subscriber: LegalizeAdulthood. LegalizeAdulthood added a comment. Why not supply a fixit that removes the cast? Repository: rL LLVM http://reviews.llvm.org/D16310 ___ cfe-commits mailing list cfe-commits@lists.llvm.org