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 still seems unlikely for / and %.
I thought the whole point was to flag instances where the
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
m...@evidente.se
>
> www.evidente.se
>
>
> Från: Richard [legal...@xmission.com]
> Skickat: den 10 februari 2016 07:28
> Till: Daniel Marjamäki; aaron.ball...@gmail.com; ale...@google.com
> Kopia: legal...@xmission.com; eugene.zele...@gmail.com;
> c
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(
+
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(
+
This revision was automatically updated to reflect the committed changes.
Closed by commit rL260223: [clang-tidy] Add 'misc-misplaced-widening-cast'
check. (authored by danielmarjamaki).
Changed prior to commit:
http://reviews.llvm.org/D16310?vs=47182&id=47310#toc
Repository:
rL LLVM
http:/
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 @@
+ has(C
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 -DLLVM_ENABLE_SPHINX
danielmarjamaki added inline comments.
Comment at: clang-tidy/misc/MisplacedWideningCastCheck.cpp:79
@@ +78,3 @@
+
+void MisplacedWideningCastCheck::check(const MatchFinder::MatchResult &Result)
{
+ const auto *Cast = Result.Nodes.getNodeAs("Cast");
I looked in
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
clang-tidy/misc/MisplacedWi
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
clang-tidy/mis
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 combi
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("+"), has
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
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
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
Comme
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
clang-tidy/misc/Mispl
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
cfe-commits@lis
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
___
cfe-commit
danielmarjamaki marked 13 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
danielmarjamaki removed rL LLVM as the repository for this revision.
danielmarjamaki updated this revision to Diff 45854.
danielmarjamaki added a comment.
Fixed review comments
http://reviews.llvm.org/D16310
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-ti
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
danielmarjamaki added inline comments.
Comment at: clang-tidy/misc/LongCastCheck.cpp:97
@@ +96,3 @@
+
+ if (!CastType->isIntegerType() || !SubType->isIntegerType())
+return;
LegalizeAdulthood wrote:
> danielmarjamaki wrote:
> > LegalizeAdulthood wrote:
> > >
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
> >
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
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:
> >
> > > W
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 casti
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
s
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
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 limi
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 as
danielmarjamaki marked an inline comment as done.
Comment at: clang-tidy/misc/LongCastCheck.cpp:43
@@ +42,3 @@
+
+static unsigned getMaxCalculationWidth(ASTContext &C, const Expr *E) {
+ E = E->IgnoreParenImpCasts();
LegalizeAdulthood wrote:
> Prefer anonymous na
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:
> >
> > > W
LegalizeAdulthood added inline comments.
Comment at: clang-tidy/misc/LongCastCheck.cpp:43
@@ +42,3 @@
+
+static unsigned getMaxCalculationWidth(ASTContext &C, const Expr *E) {
+ E = E->IgnoreParenImpCasts();
Prefer anonymous namespace over `static` to scope visib
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 diff
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 check
danielmarjamaki marked an inline comment 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
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
clang-tidy/misc/LongCastChec
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 * (long)1000
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
h
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
h
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
-
42 matches
Mail list logo
