[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Great job!  Thank you for your work!

I would say, fixing a couple of minor style issues and testing it on a few more projects, and it is ready to land!

> Another point is that I don't know how to get printed stats from the scan-build.

That option in `scan-build` is called `-internal-stats`.

> I completely agree with you. But, unfortunately, vim-proj is the only I could squeeze from that bunch.

My opinion here is that any projects (with a good amount of users or stars on GitHub ^-^ ) will do.  If it is hard to build them on Windows, move to the next project.  I guess projects with a minimal number of dependencies should be fairly easy on any platform.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:80
+
+  static size_t IndexFromOp(BinaryOperatorKind OP) {
+    return static_cast<size_t>(OP - BO_LT);
----------------
ASDenysPetrov wrote:
> vsavchenko wrote:
> > I would prefer function names to comply with LLVM coding standards (start with a verb and a lowercase letter
> > https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> I'll add a **get **prefix. Saying about lower/uppercases, I'd say this is a mess inside this file. I've just decided to use an upper one.
I agree that it is a mess, but if we want to reduce it with time, I'd say we should stick to the official style guide.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:689
+
+    // Loop goes through the all columns except the last `UnknownX2`
+    // We treat `UnknownX2` column separately at the end of the loop body.
----------------
ASDenysPetrov wrote:
> vsavchenko wrote:
> > Sorry for nitpicking, but it seems like the grammar is a bit off in the **the all columns except the last** part
> Do you mean to change to **all of the columns exept the last one ('UnknownX2')**?
Yep, sounds good!


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:696
+      const SymSymExpr *SymSym = SymMgr.getSymSymExpr(LHS, QueriedOP, RHS, T);
+      const RangeSet *RangeSet = State->get<ConstraintRange>(SymSym);
+
----------------
ASDenysPetrov wrote:
> vsavchenko wrote:
> > Maybe we can name this constant somehow differently to be more verbose? 
> What do you think about **FoundRangeSet**?
I was thinking about adding more context to the name, something like `RelatedRangeSet` or similar.  To show not only what it is (`RangeSet`) and how we got it (`found`), but also its purpose or meaning.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:700-705
+      if (!RangeSet) {
+        const BinaryOperatorKind ROP =
+            BinaryOperator::reverseComparisonOp(QueriedOP);
+        SymSym = SymMgr.getSymSymExpr(RHS, ROP, LHS, T);
+        RangeSet = State->get<ConstraintRange>(SymSym);
+      }
----------------
ASDenysPetrov wrote:
> vsavchenko wrote:
> > Please, correct me if I'm wrong, but I thought that we are iterating over //all// comparison operators (excluding `<=>`), which means that if we don't find it on this iteration for let's say `x < y` then we'll find it (or already did) for `x > y`.  So, my question is - why do we have this clause then?
> > 
> > And it confuses me that `RangeSet` now corresponds to a //reversed// operator, while `QueriedOP` remains the same.
> > 
> > Maybe a good commentary explaining why we need it could help!
> No-no. Please, look carefully.
> At first we try to find `X op Y`, when **op** is a comparison operator. If we failed then try to find the reversed (flipped) version of the expression `Y reversed(op) X`.
> Examples: `x > y and y < x`, `x != y and y != x`, `x <= y and y >= x`
> 
> We don't care about operand positions, we just want to find any of its previous relation, and which branch of it we are now in.
Oh, I see now. I missed the part where `RHS` is on the left now :-) 


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

https://reviews.llvm.org/D78933