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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
