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