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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:32
+                    BO_GE < BO_EQ && BO_EQ < BO_NE,
+                "This class relies on operators order. Rework it otherwise.");
+
----------------
+1 for this static assert!
It's good to ensure such assumptions, even if those are very unlikely to change


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


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:665
+
+    auto SSE = dyn_cast<SymSymExpr>(Sym);
+    if (!SSE)
----------------
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`


================
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.
----------------
Sorry for nitpicking, but it seems like the grammar is a bit off in the **the all columns except the last** part


================
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) {
----------------
I think that **treat** is not the best option here. Maybe smith like **process** will do?


================
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);
+
----------------
Maybe we can name this constant somehow differently to be more verbose? 


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


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

https://reviews.llvm.org/D78933