[PATCH] D99797: [analyzer] Implemented RangeSet::Factory::unite function to handle intersections and adjacency

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:149
+
+RangeSet RangeSet::Factory::unite(RangeSet Original, llvm::APSInt Point) {
+  return unite(Original, Range(ValueFactory.getValue(Point)));
----------------
ASDenysPetrov wrote:
> steakhal wrote:
> > Why do you take `APSInt`s by value? Generally, we take them by reference.
> I want to send a message to the caller that he can pass an arbitrary **APSInt** without warrying about making it permanent (aka stored by the Factory). But we can revise this contract and carry this responsibility to a caller.
> Why do you take `APSInt`s by value? Generally, we take them by reference.

Actually, it is specific to `BasicValueFactory` to cache the `APSInt`s, however, it might not be the best practice. I doubt that somebody has ever measured the performance of passing APSInts by value, so my guess is the caching of `APSInt`s might be an early optimization that might be more harmful than advantageous. On top of all this, we do the caching inconsistently, just consider the member functions of `APSIntType`, they all return by value.

Perhaps (totally independently from this patch of course), it might be worth to have a measurement/comparison with removed cache and pass by value.



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

https://reviews.llvm.org/D99797