[PATCH] D55734: [analyzer] Revise GenericTaintChecker's internal representation

[Borsik Gábor via Phabricator via cfe-commits]

boga95 marked 4 inline comments as done.
boga95 added a comment.

Is it ready to land?



================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:212
+      llvm::StringSwitch<TaintPropagationRule>(Name)
+          .Case("atoi", TaintPropagationRule({0}, {ReturnValueIndex}))
+          .Case("atol", TaintPropagationRule({0}, {ReturnValueIndex}))
----------------
NoQ wrote:
> `llvm::ArrayRef` has a fancy feature: it can be constructed not only from an initializer list, but also from a single element. This could have allowed skipping `{}` in these constructors when there is just one value in the list. That's what i would have done, but i guess it's up to you to decide which approach is prettier.
Those who don't know that feature will be confused. I should make changes in the constructor to support it. I'm going to leave it.


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:224
+          .Case("read", TaintPropagationRule({0, 2}, {1, ReturnValueIndex}))
+          .Case("pread", TaintPropagationRule({0, 2, 3}, {1, ReturnValueIndex}))
+          .Case("gets", TaintPropagationRule({}, {0, ReturnValueIndex}))
----------------
NoQ wrote:
> Now `pread` doesn't produce a tainted result when only the buffer is tainted but other arguments are not. Is it a functional change? I guess we should be able to add tests for it. You can also commit this patch with `{0, 1, 2, 3}` and remove `1` in the next patch that also adds a test.
I changed it to `{0, 1, 2, 3}`. I don't want to make any change in the behavior in this patch. I think who made it use InvalidArgIndex because the previous implementation wasn't expressive enough.


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

https://reviews.llvm.org/D55734