[PATCH] D55734: [analyzer] Refactoring GenericTaintChecker.cpp

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Hmm, I find the revision title and summary a little vague -- I'd expect a revision called "Refactoring" not to feature any funcitonal change, yet you changed how variadic functions are handled. Please

- Keep purely formatting changes to your earlier revision, and rebase this patch on that
- Edit the revision title and/or summary about what your patch does,
- If it features any change in the checker's behavior, include tests.



================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:125
+    unsigned VariadicIndex;
+    /// Show when a function has variadic parameters. If it has, it mark all
+    /// of them as source or destination.
----------------
typo: marks


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:132-137
+    TaintPropagationRule(std::initializer_list<unsigned> &&Src,
+                         std::initializer_list<unsigned> &&Dst,
+                         VariadicType Var = VariadicType::None,
+                         unsigned VarIndex = InvalidArgIndex)
+        : SrcArgs(std::move(Src)), DstArgs(std::move(Dst)),
+          VariadicIndex(VarIndex), VarType(Var) {}
----------------
I like this //a lot//! Call sites look very nice.


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:145-158
     inline void addSrcArg(unsigned A) { SrcArgs.push_back(A); }
-    inline void addDstArg(unsigned A)  { DstArgs.push_back(A); }
+    inline void addDstArg(unsigned A) { DstArgs.push_back(A); }
 
-    inline bool isNull() const { return SrcArgs.empty(); }
+    inline bool isNull() const {
+      return SrcArgs.empty() && DstArgs.empty() &&
+             VariadicType::None == VarType;
+    }
----------------
In-class method definitions are implicitly `inline` -- could you please remove them while we're cleaning up anyways? :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55734/new/

https://reviews.llvm.org/D55734