[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Just a few nits left.
Consider marking 'done' the corresponding boxes.



================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:211
+    // Check if the negation of the RHS is representable:
+    // * if resultTy is unsigned, then negation is always representable
+    // * if resultTy is signed, and RHS is not the lowest representable
----------------
It feels odd that the comments refer to `resultTy`, but you then use `resultIntTy` instead, and both of these entities are alive at this scope.


================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:215
+    if (resultIntTy.isUnsigned() ||
+        (ConvertedRHSValue > resultIntTy.getMinValue())) {
+      ConvertedRHS = &BasicVals.getValue(-ConvertedRHSValue);
----------------
These should be the same.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124658/new/

https://reviews.llvm.org/D124658