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