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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov marked 6 inline comments as done.
ASDenysPetrov added a comment.

Do not apologize for syntax, language or code style remarks. Them make code better and it's really important for me to know about my faults.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:80
+
+  static size_t IndexFromOp(BinaryOperatorKind OP) {
+    return static_cast<size_t>(OP - BO_LT);
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:665
+
+    auto SSE = dyn_cast<SymSymExpr>(Sym);
+    if (!SSE)
----------------
vsavchenko wrote:
> I believe that when we use auto we still try to be more verbose with it, so in this case it should be smth like `const auto *SSE`
I'll do.


================
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.
----------------
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')**?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:690
+    // Loop goes through the all columns except the last `UnknownX2`
+    // We treat `UnknownX2` column separately at the end of the loop body.
+    for (size_t i = 0; i < CmpOpTable.GetCmpOpCount(); ++i) {
----------------
vsavchenko wrote:
> I think that **treat** is not the best option here. Maybe smith like **process** will do?
I met a lot of **treat ** usage in terms of **handle/process** among the code, so just followed it.


================
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);
+
----------------
vsavchenko wrote:
> Maybe we can name this constant somehow differently to be more verbose? 
What do you think about **FoundRangeSet**?


================
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);
+      }
----------------
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.


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

https://reviews.llvm.org/D78933