[PATCH] D11700: Added remove taint support to ProgramState.

[Ted Kremenek via cfe-commits]

krememek added inline comments.

================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:444-448
@@ +443,7 @@
+
+  SymbolRef
+  getSymbolFromStmt(const Stmt *S, const LocationContext *LCtx) const;
+
+  const MemRegion*
+  getRegionFromStmt(const Stmt *S, const LocationContext *LCtx) const;
+
----------------
Can we add documentation comments for these?  Seems like generally useful utility methods.  We could also probably just make these public.

================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:653-654
@@ -654,3 +652,4 @@
+                                              *LCtx) const {
   if (const Expr *E = dyn_cast_or_null<Expr>(S))
     S = E->IgnoreParens();
 
----------------
Is this even needed?  I think Environment::getSVal() already handles parenthesis and other ignored expressions.  This looks like dead code.

This can probably just be an inline method in ProgramState.h, that just forwards to getSVal(S, LCtx).getAsSymbol().

Alternatively, if this is only called once, we don't need to add a method at all, since it is just a one liner.

================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:660-663
@@ +659,6 @@
+
+const MemRegion* ProgramState::getRegionFromStmt(const Stmt *S, const LocationContext
+                                                     *LCtx) const {
+  return getSVal(S, LCtx).getAsRegion();
+}
+
----------------
This is just a one-liner.  Do we really need this method at all?  It is only called once.

================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:672-676
@@ -660,7 +671,7 @@
 
-  const MemRegion *R = getSVal(S, LCtx).getAsRegion();
+  const MemRegion *R = getRegionFromStmt(S, LCtx);
   addTaint(R, Kind);
 
   // Cannot add taint, so just return the state.
   return this;
 }
----------------
This looks fishy.  'addTaint' returns a new state, but then the return value is ignored, and 'this' is returned.

================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:704-708
@@ +703,7 @@
+
+  const MemRegion *R = getRegionFromStmt(S, LCtx);
+  removeTaint(R, Kind);
+
+  // Cannot remove taint, so just return the state.
+  return this;
+}
----------------
This looks fishy.  'removeTaint' returns a new state, but then the return value is ignored.  'ProgramState' values are immutable, so this method appears to do nothing.


http://reviews.llvm.org/D11700