[PATCH] D88359: [analyzer][RFC] Complete rewrite of ArrayBoundCheckerV2

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

steakhal added a comment.

Thank you for your time @balazske!
Your catches were really valuable.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:61
+
+  return NeedsExtraBitToPreserveSigness ? Signed : ExtendedSigned;
+}
----------------
balazske wrote:
> Should not the `ExtendedSigned` be in the true branch?
You are right. Thanks.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:87
+/// stored value.
+static APSInt absWithoutWrapping(const llvm::APSInt &Value) {
+  // If unsigned, we might need a sign bit.
----------------
balazske wrote:
> I do not like these function names. `getCommonSignedTypeToFit`, `isRepresentableBySigned`, `getAbsWithoutWrapping` is better to have the common "start verbs" in the names.
You are probably right about that.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:241
+                              const APSInt &ExclusiveUpperBound)
+      : LastValidState(std::move(State)), SVB(SVB), RootSymbol(RootSymbol) {
+    assert(LastValidState);
----------------
balazske wrote:
> std::move is not needed here
But definitely not hurt :D
I quite like to use `move` on by-value parameters.
This way the implementation could omit the increment decrement even if the implementation currently doesn't exploit this.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:269
+  /// Starts the simplification process.
+  SimplificationResult Simplify() {
+    LLVM_DEBUG(llvm::dbgs() << __func__ << ": initially: '";
----------------
balazske wrote:
> Function name should start with lower case.
Thanks.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:285
+               LastValidState->printJson(llvm::dbgs()); llvm::dbgs() << '\n';);
+    return {std::move(LastValidState), std::move(LowerBound),
+            std::move(UpperBound), SymbolVal(RootSymbol), SimplificationFailed};
----------------
balazske wrote:
> The moves here are not needed.
I'm not convinced about that.
I think without move we would do a copy there - which could be expensive in the case of `APSInt`s.
Here is an example demonstrating this: https://godbolt.org/z/E5dqWb
I know that currently APSInt does not support move operations, but I already plan adding that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88359