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

[Francisco via cfe-commits]

franchiotta 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;
+
----------------
krememek wrote:
> krememek wrote:
> > Can we add documentation comments for these?  Seems like generally useful utility methods.  We could also probably just make these public.
> Actually, I'm wondering if we really need to add these at all.  They are just one liners that easily could be written where they are used.
Right. Removing these methods, and adding the one-liners directly where they are used.

================
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();
 
----------------
krememek wrote:
> 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.
Yes, you are right. It is not needed since it is handle by ignoreTransparentExprs method in Environment module. I will not add this method at all.

================
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();
+}
+
----------------
krememek wrote:
> This is just a one-liner.  Do we really need this method at all?  It is only called once.
We don't. I will add the one-liner directly where it is used.

================
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;
 }
----------------
krememek wrote:
> This looks fishy.  'addTaint' returns a new state, but then the return value is ignored, and 'this' is returned.
Yes, it does.. I will return at the time the last addTaint is invoked.

================
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;
+}
----------------
krememek wrote:
> 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.
Yes, you are right. I will return at the time the last addTaint is invoked.


http://reviews.llvm.org/D11700