[PATCH] D84736: [analyzer][RFC] Handle pointer difference of ElementRegion and SymbolicRegion
Valeriy Savchenko via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 5 00:52:15 PDT 2020
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
More information about the cfe-commits
mailing list