[PATCH] D84736: [analyzer][RFC] Handle pointer difference of ElementRegion and SymbolicRegion

[Valeriy Savchenko via Phabricator via cfe-commits]

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