ASDenysPetrov marked 3 inline comments as done.
ASDenysPetrov added a comment.

> The measurements I mentioned earlier about runtimes, node count and so on 
> could help answer some of those questions.

I'm looking for an appropriate project to measure performance, since I have 
some troubles running the test-suite on Windows.



================
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
----------------
xazax.hun wrote:
> 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.
I'll try to come up how to turn it into the class.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:613
+
+  auto OP = SSE->getOpcode();
+
----------------
xazax.hun wrote:
> 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.
Thanks for the guidance, I'll check the code for implicit types.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:668
+
+    auto BranchState = CmpOpTable[IndexSymOP][IndexPrevOP];
+
----------------
xazax.hun wrote:
> 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`.
No problem. I'll rename them.


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

Reply via email to