[PATCH] D59516: [analyzer] Add custom filter functions for GenericTaintChecker

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

Szelethus added a comment.

In general, the patch is looking alright, I'll take a second look later on. Don't mind my inlines too much, they are more directed towards the original code then your changes.



================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:53-56
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    ID.AddInteger(Arg);
+    ID.AddInteger(TagType);
+  }
----------------
Interesting, isn't this predefined for `std::pair`?


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:474-476
+  const FunctionDecl *FDecl = C.getCalleeDecl(CE);
+  if (!FDecl || FDecl->getKind() != Decl::Function)
+    return;
----------------
When do these happen? For implicit functions in C? 


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:478-480
+  StringRef Name = C.getCalleeName(FDecl);
+  if (Name.empty())
+    return;
----------------
And this? Lambdas maybe?


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:560
 
-  for (unsigned ArgNum : TaintArgs) {
+  for (auto ArgTypePair : TaintArgs) {
+    unsigned ArgNum = ArgTypePair.Arg;
----------------
Please add the full type name, else it isn't obvious why we don't take it as a const reference.


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:561-562
+  for (auto ArgTypePair : TaintArgs) {
+    unsigned ArgNum = ArgTypePair.Arg;
+    TaintTagType TagType = ArgTypePair.TagType;
     // Special handling for the tainted return value.
----------------
We don't use `std::pair`, so we have descriptive field names, do we need these?


================
Comment at: lib/StaticAnalyzer/Checkers/Taint.h:25-28
 using TaintTagType = unsigned;
 
-static constexpr TaintTagType TaintTagGeneric = 0;
+static constexpr TaintTagType TaintTagNotTainted = 0;
+static constexpr TaintTagType TaintTagGeneric = 1;
----------------
Is there a **very** good reason behind us not using an `enum` instead?


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

https://reviews.llvm.org/D59516