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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@vsavchenko Thanka for the suggestions! I'll take them into account and update the patch.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:250
+    /// guarantee this.
+    ContainerType unite(const ContainerType &LHS, const ContainerType &RHS);
 
----------------
vsavchenko wrote:
> `ContainerType` is basically a mutable version of `RangeSet`, so there is only one reason to return it - you believe that the users might want to modify it after they called this `unite`.  But as long as this `unite` is just a generalized version of user-facing `unites, it can totally return `RangeSet`.
I'm going to use raw ContainerType in further patches. So this is exactly what I want.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:221
+  Result.append(B, E);
+
+  return Result;
----------------
vsavchenko wrote:
> Oof, I don't know about this algorithm.  I mean it does its job.  But IMO it lacks a good description of what are the invariants and what are the different situations we are looking for.
> Aaaand you kind of re-check certain conditions multiple times.  One example here is the check for `Min` and `Max`. Those situations are super rare, but we check for them on every single iteration. `std::min` and `std::max` are additional comparisons.  As I mentioned before, constant factor is the key here and less comparisons we do is way more important than doing binary search at some point.
> Just make a benchmark if you don't believe me (with google-benchmark, for example).  The version with less comparisons will dominate one with more on `RangeSet` under 20 (and they'll be even smaller in practice).
I'll investigate the whole algorithm once more and reduce comparisons.


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

https://reviews.llvm.org/D99797