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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
