[PATCH] D14203: [analyzer] Improve pointer arithmetic checker.

[Devin Coughlin via cfe-commits]

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