xazax.hun marked an inline comment as done.
xazax.hun added a comment.
Thanks!
I still have some nits inline, but overall the implementation looks good to me.
I think, however, that we should not only focus on the implementation but on
the high-level questions.
Is this the way forward we want? Can we do it more efficiently? What is the
performance penalty of this solution?
The measurements I mentioned earlier about runtimes, node count and so on could
help answer some of those questions.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:587
+ // !=|1 |1 |* |* |0 |1 |0 |
+ // Columns stands for a preceding operator.
+ // Rows stands for a current operator.
----------------
I think it would be better to use slightly different terminology. Instead of
preceding operator I would say something like "previously known fact".
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:597
+ static constexpr size_t CmpOpCount = BO_NE - BO_LT + 1;
+ static const TriState CmpOpTable[CmpOpCount][CmpOpCount + 1] = {
+ // < > <= >= == != UnknownX2
----------------
I think this table together with the operations that transform between indices
and operators could be factored out into a separate class. That would make this
method slightly cleaner.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:613
+
+ auto OP = SSE->getOpcode();
+
----------------
In LLVM we usually only use `auto` for iterators and to avoid repeating the
type twice. Auto is ok above with the `dynamic_cast`, but here and some other
places the type should be spelled out.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:668
+
+ auto BranchState = CmpOpTable[IndexSymOP][IndexPrevOP];
+
----------------
I think `IndexPrevOP` is confusing as the "previous" operation is not
necessarily the previous operation we evaluated. In fact `IndexPrevOP` could
have been evaluated any time in the past. We query the known facts that are
known for the current state, I think this is what the names should reflect. I
suggest to use `CurrentOP` and `QueriedOP`.
================
Comment at: clang/test/Analysis/constraint_manager_conditions.cpp:184
+}
\ No newline at end of file
----------------
ASDenysPetrov wrote:
> xazax.hun wrote:
> > Nit: please add new line at the end of the file.
> I've added a new line and checked twice before creating a patch. But it
> didn't appear.
This looks good now. If it misses the new line there is a message saying that
explicitly.
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
