vsavchenko added a comment.
Great job, I'm on board with this change!
I think that our code is a bit under commented when it comes to the logic
inside of functions, so I would really appreciate if we can add a good chunk of
comments here!
Also, I believe integration tests would be pretty much straightforward with
`clang_analyzer_eval`.
================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1035-1038
+ return evalBinOpNN(state, BO_Mul, Index,
+ makeArrayIndex(SingleElementSize.getQuantity()),
+ ArrayIndexTy)
+ .castAs<NonLoc>();
----------------
I would prefer having a comment for this line with a very simple formula of the
thing we are calculating. I think it can increase the readability of the code
because when there is a call accepting a whole bunch of arguments it is never
obvious and it always takes time to parse through.
================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1041
+
+ // If both has the same memory region, and the index has a concrete
value,
+ // we can evaluate equality operators.
----------------
This comment is a bit deceiving IMO. It returns a concrete value even when
`HasSameMemRegions` is false, but from the comment it seems like we evaluate
the operator ONLY when `HasSameMemRegions` is true.
================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1068
+ }
+ } else if (LeftER && !RightER) {
+ NonLoc LeftIndex = ByteOffsetOfElement(LeftER);
----------------
I think it's better to add comments for each case describing what is the
situation we handle here in a simplified form.
And for each return within the case - a short comment with the reason.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84736/new/
https://reviews.llvm.org/D84736
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
