dcoughlin added a comment.
Gabor,
This is an alpha checker. Do you anticipate turning it on by default?
Comments inline.
================
Comment at: lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp:28
@@ -24,1 +27,3 @@
namespace {
+enum class AllocKind {
+ SingletonNew,
----------------
Is it necessary to distinguish so many cases here? For example, why do we need
to distinguish between PlacementNew and OverloadedNew?
Another thought: given the expense of tracking stuff in the GDM could we
instead track whether pointer arithmetic is explicitly disallowed for a given
region? Then we wouldn't have to track any data for the "good" pointers.
Also, how does AllocKind relate to the AllocationFamily from MallocChecker?
Could that checker's state be used so we don't have to track any additional
information here?
If you do keep AllocKind, I think it would be good to add a comment describing
how this enum is used and the intended meaning of each element.
================
Comment at: lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp:29
@@ +28,3 @@
+enum class AllocKind {
+ SingletonNew,
+ ArrayNew,
----------------
I'm not sure "singleton" is the right term here. I associate "singleton" with
the design pattern of only having a single instance of a class allocated at a
given time (a form of global shared state). Also, SingletonMalloc doesn't
appear to be used anywhere.
================
Comment at: lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp:88
@@ -28,1 +87,3 @@
+ mutable std::unique_ptr<BuiltinBug> BT_polyArray;
+ mutable llvm::SmallVector<IdentifierInfo *, 8> AllocFunctions;
----------------
I think it would be better to use llvm::SmallSet here.
================
Comment at: lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp:137
@@ -43,2 +136,3 @@
- const MemRegion *LR = LV.getAsRegion();
+const MemRegion *PointerArithChecker::getArrayRegion(const MemRegion *Region,
+ bool &Polymorphic,
----------------
I think it would be good to add a doc comment for this function describing what
the function does and its parameters as well as whether they are input or
output parameters.
I also wonder if this logic is better expressed as a loop rather than
recursion. What do you think?
================
Comment at: lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp:150
@@ +149,3 @@
+ return getArrayRegion(Region, Polymorphic, AKind, C);
+ default:
+ break;
----------------
In general, I think it is better to avoid default in cases like these so that
when an enum case is added the compiler issues a warning and thus forces the
person adding the change to think about what the behavior of the new case
should be.
================
Comment at: lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp:198
@@ +197,3 @@
+ this, "Dangerous pointer arithmetic",
+ "Pointer arithmetic does not account for polymorphic object sizes "
+ "and attempting to perform pointer arithmetic on a polymorphic "
----------------
I think "polymorphic" is a bit jargony. Can this diagnostic be explained in
terms of base and derived classes?
================
Comment at: test/Analysis/ptr-arith.cpp:47
@@ +46,3 @@
+ p = p + 2; // expected-warning{{}}
+ p = 2 + p; // expected-warning{{}}
+ p += 2; // expected-warning{{}}
----------------
I think it would be good to add tests showing `p = i*0 + p' and `p = p + i*0'
don't alarm. Also `p += i*0'.
http://reviews.llvm.org/D14203
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
