[PATCH] D28445: [Analyzer] Extend taint propagation and checking

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Yeah, i think this is now as easy as i expected it to be :)

Still, the new API is in need of better documentation, because the notion of default binding is already confusing, and the new use-case we now have for this API is even more confusing. I don't instantly see a good way to justify this hack, but some day we'd need to either do that or refactor further. The notion of default binding needs to become more "material".

A more expanded doxygen comment should probably be a great start.



================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/Store.h:66
+  /// \return The default value bound to the LazyCompoundVal \c lcv.
+  virtual SVal getDefaultBinding(Store store, nonloc::LazyCompoundVal lcv) = 0;
+
----------------
I think there are two different use cases here:
1. `getDefaultBinding(Store, Region)` would retrieve default binding in a given store for the region's base region,
2. `getDefaultBinding(LazyCompoundVal)` would retrieve default binding for the lazy compound value's base region from the lazy compound value's store - a shorthand for `getDefaultBinding(lcv.getStore(), lcv.getRegion())`.

Otherwise we're passing two different stores into the function (one directly, another as part of the lazy compound value), and it's confusing which one is actually used.




================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:498
+  SVal getDefaultBinding(Store S, nonloc::LazyCompoundVal L) override {
+    if (!L.getRegion())
+      return UnknownVal();
----------------
`LazyCompoundVal` always has a region.


================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:505
+
+    return UnknownVal();
+  }
----------------
Because UnknownVal is an interesting default binding, quite different from lack of default binding, i'd rather return `Optional<SVal>` from that function (and None here).


https://reviews.llvm.org/D28445