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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


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


================
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.
----------------
I do not like these function names. `getCommonSignedTypeToFit`, `isRepresentableBySigned`, `getAbsWithoutWrapping` is better to have the common "start verbs" in the names.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:241
+                              const APSInt &ExclusiveUpperBound)
+      : LastValidState(std::move(State)), SVB(SVB), RootSymbol(RootSymbol) {
+    assert(LastValidState);
----------------
std::move is not needed here


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


================
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};
----------------
The moves here are not needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88359