NoQ added a comment.
Herald added a subscriber: Charusso.
Herald added a project: clang.
Hey, i'm seeing a crash in this checker, would you like to look at it? It looks
as if you're not being careful about dereferences/lvalue-to-rvalue-casts so it
tries to compare `` to `e1`.
**$ `cat
This revision was automatically updated to reflect the committed changes.
Closed by commit rC347513: [analyzer] INT50-CPP. Do not cast to an out-of-range
enumeration checker (authored by Szelethus, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D33672?vs=172516=175154#toc
Szelethus added a comment.
Herald added a subscriber: baloghadamsoftware.
I'll commit tomorrow, when I have time to keep an eye out for buildbots. Thanks!
https://reviews.llvm.org/D33672
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
aaron.ballman added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
NoQ wrote:
>
gamesh411 added a comment.
Hi!
Thanks for your reviews, although I haven't been active for some time now.
I personally do not have commit rights, so could someone else take care of it?
https://reviews.llvm.org/D33672
___
cfe-commits mailing list
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
aaron.ballman wrote:
> NoQ wrote:
>
aaron.ballman added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
NoQ wrote:
>
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
aaron.ballman wrote:
> Szelethus
aaron.ballman added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
Szelethus wrote:
>
Szelethus added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
Szelethus wrote:
> ZaMaZaN4iK
Szelethus added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
ZaMaZaN4iK wrote:
>
ZaMaZaN4iK updated this revision to Diff 172516.
ZaMaZaN4iK added a comment.
Fix using `auto` in case where it leads to worse readability
https://reviews.llvm.org/D33672
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
ZaMaZaN4iK added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
lebedev.ri wrote:
>
lebedev.ri added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
ZaMaZaN4iK wrote:
>
ZaMaZaN4iK added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
lebedev.ri wrote:
>
lebedev.ri added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
ZaMaZaN4iK wrote:
>
ZaMaZaN4iK added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
lebedev.ri wrote:
>
lebedev.ri added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
ZaMaZaN4iK wrote:
>
ZaMaZaN4iK updated this revision to Diff 172515.
ZaMaZaN4iK added a comment.
1. Fix indentation in test file
2. Return back capitalization for the checker description
https://reviews.llvm.org/D33672
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
ZaMaZaN4iK added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96
+ // Get the value of the expression to cast.
+ const auto ValueToCastOptional =
+ C.getSVal(CE->getSubExpr()).getAs();
aaron.ballman wrote:
>
aaron.ballman added a comment.
In https://reviews.llvm.org/D33672#1283778, @ZaMaZaN4iK wrote:
> Which other changes and/or approvals are required for merging into trunk?
There's one unresolved comment from earlier and a few very minor nits that I
found, but I think this is ready to land
ZaMaZaN4iK added a comment.
Which other changes and/or approvals are required for merging into trunk?
https://reviews.llvm.org/D33672
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Szelethus added a comment.
In https://reviews.llvm.org/D33672#1279520, @ZaMaZaN4iK wrote:
> Thank you for the roadmap! Honestly I am not so familiar with Clang AST and
> Clang Static Analyzer details (all my experience is writing some simple
> checkers for clang-tidy and watching some videos
ZaMaZaN4iK added a comment.
In https://reviews.llvm.org/D33672#1279305, @NoQ wrote:
> Thanks! I like where this is going. Let's land the patch and continue
> developing it incrementally in trunk.
>
> The next steps for this checker are, in my opinion:
>
> - Do the visitor thingy that i
Szelethus added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:36
+ const ProgramStateRef PS;
+ SValBuilder
+
NoQ wrote:
> ZaMaZaN4iK wrote:
> > Szelethus wrote:
> > > You can acquire `SValBuilder` from `ProgramState`:
NoQ accepted this revision.
NoQ added a comment.
Thanks! I like where this is going. Let's land the patch and continue
developing it incrementally in trunk.
The next steps for this checker are, in my opinion:
- Do the visitor thingy that i requested in inline-311373
ZaMaZaN4iK added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:36
+ const ProgramStateRef PS;
+ SValBuilder
+
Szelethus wrote:
> You can acquire `SValBuilder` from `ProgramState`:
>
ZaMaZaN4iK updated this revision to Diff 171506.
ZaMaZaN4iK marked 2 inline comments as done.
ZaMaZaN4iK added a comment.
Fix typedef -> using
https://reviews.llvm.org/D33672
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
ZaMaZaN4iK updated this revision to Diff 171503.
ZaMaZaN4iK added a comment.
Add new test for enum bit field
https://reviews.llvm.org/D33672
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
aaron.ballman added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:19
+// of casting an integer value that is out of range
+//===--===//
+
ZaMaZaN4iK
ZaMaZaN4iK marked 3 inline comments as done.
ZaMaZaN4iK added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:19
+// of casting an integer value that is out of range
ZaMaZaN4iK updated this revision to Diff 171472.
ZaMaZaN4iK added a comment.
Add the reference to CERT page
https://reviews.llvm.org/D33672
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
Szelethus added a comment.
I think is good to go! Please wait on @xazax.hun or @NoQ to have the final say
(it's been a while since this revision was accepted by them), but for a
work-in-progress alpha checker, I like what I'm seeing.
Comment at:
ZaMaZaN4iK updated this revision to Diff 171443.
https://reviews.llvm.org/D33672
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
test/Analysis/enum-cast-out-of-range.cpp
Szelethus added a comment.
Please reupload with full context.
(https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)
https://reviews.llvm.org/D33672
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ZaMaZaN4iK updated this revision to Diff 171438.
ZaMaZaN4iK added a comment.
1. Changed std::transform to llvm::transform
2. Described the check in .html file
3. Fixed RUN command for the test file
https://reviews.llvm.org/D33672
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
ZaMaZaN4iK commandeered this revision.
ZaMaZaN4iK added a reviewer: gamesh411.
ZaMaZaN4iK added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:19
+// of casting an integer value that is out of range
Szelethus added inline comments.
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:296-299
+def EnumCastOutOfRangeChecker : Checker<"EnumCastOutOfRange">,
+ HelpText<"Check integer to enumeration casts for out of range values">,
+
Szelethus added a comment.
Also needs an entry to `www/analyzer/alpha_checks.html`.
Comment at: test/Analysis/enum-cast-out-of-range.cpp:1
+// RUN: %clang_cc1 -std=c++11 -analyze
-analyzer-checker=alpha.cplusplus.EnumCastOutOfRange -verify %s
+
1. Prefer
whisperity added a comment.
Herald added subscribers: donat.nagy, Szelethus, mikhail.ramalho, rnkovacs,
szepet.
Herald added a reviewer: george.karpenkov.
//Soft ping.//
https://reviews.llvm.org/D33672
___
cfe-commits mailing list
aaron.ballman added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:19
+// of casting an integer value that is out of range
+//===--===//
+
If this check
gamesh411 updated this revision to Diff 112156.
https://reviews.llvm.org/D33672
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
test/Analysis/enum-cast-out-of-range.cpp
Index:
gamesh411 updated this revision to Diff 112154.
gamesh411 added a comment.
I have implemented the std::transform. The previous version used std::for_each
because the iterator for enum declarations was not a random access iterator,
but it turned out that I can solve this problem via
aaron.ballman added a comment.
In https://reviews.llvm.org/D33672#830492, @gamesh411 wrote:
> As for the the loss of precision problem, in the special case of char the
> size of char is known. However does the analysis have the necessary
> information in this stage to know the size of an int
gamesh411 marked 9 inline comments as done.
gamesh411 added a comment.
As for the the loss of precision problem, in the special case of char the size
of char is known. However does the analysis have the necessary information in
this stage to know the size of an int for example? I found
gamesh411 updated this revision to Diff 109546.
gamesh411 added a comment.
Applied most of the suggested changes, thanks for all the insights!
https://reviews.llvm.org/D33672
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
aaron.ballman added a comment.
In https://reviews.llvm.org/D33672#827513, @xazax.hun wrote:
> Even though it is not undefined behavior in C, it can still cause surprising
> behavior for the users. I think maybe putting it into the optin package
> instead of cplusplus is better. What do you
xazax.hun added a comment.
Even though it is not undefined behavior in C, it can still cause surprising
behavior for the users. I think maybe putting it into the optin package instead
of cplusplus is better. What do you think?
https://reviews.llvm.org/D33672
xazax.hun added a comment.
Even though it is not undefined behavior in C, it can still cause surprising
behavior for the users. I think maybe putting it into the optin package instead
of cplusplus is better. What do you think?
https://reviews.llvm.org/D33672
aaron.ballman added a comment.
In https://reviews.llvm.org/D33672#819683, @xazax.hun wrote:
> Aaron, could you comment on the applicability of this check to C? Thanks in
> advance.
C has different rules for their enumerations in that the enumerators are all
ints, but the enumeration type is
xazax.hun added a reviewer: aaron.ballman.
xazax.hun added a comment.
Aaron, could you comment on the applicability of this check to C? Thanks in
advance.
https://reviews.llvm.org/D33672
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
NoQ accepted this revision.
NoQ added a comment.
These comments of mine are mostly random ideas for follow-up patches, i agree
with Gabor this patch is good to land.
https://reviews.llvm.org/D33672
___
cfe-commits mailing list
NoQ added a comment.
Did the checker flag any intentionally underpopulated enums, eg.
enum {
Unset = -1
} NumberOfChickens = 15;
(such code is bad, but it is not necessarily buggy - as far as i understand
from the CERT rule, we're talking about "unspecified behavior" which some
NoQ added a comment.
Yeah, this looks good. How does this compare to `alpha.core.BoolConversion`,
should we maybe merge them?
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:84
+void EnumCastOutOfRangeChecker::reportWarning(CheckerContext ) const {
+ if
xazax.hun accepted this revision.
xazax.hun added reviewers: dcoughlin, NoQ, a.sidorin.
xazax.hun added a comment.
This revision is now accepted and ready to land.
Looks good to me. Did you run it on a codebase to check the results?
https://reviews.llvm.org/D33672
gamesh411 marked 2 inline comments as done.
gamesh411 added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:104
+ // Check whether the cast type is an enum.
+ const auto ED = dyn_cast_or_null(T->getAsTagDecl());
+ // If it is not an
gamesh411 updated this revision to Diff 107886.
gamesh411 added a comment.
Fixed the naming convention issues. Also applied the suggested modifications
inside the overridden checker method.
https://reviews.llvm.org/D33672
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
57 matches
Mail list logo
