donat.nagy added a comment.
@steakhal Thanks for the review, it was really instructive :). I'll probably
upload the updated commit on Monday.
What kind of measurements are you suggesting? I analyzed a few open source
projects with the patched Clang SA (see results in my previous commit) and a
few additional analysis runs are still ongoing. (I didn't measure
performance/runtime, because I don't think that this change significantly
affects those.)
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:167-168
- NonLoc rawOffsetVal = rawOffset.getByteOffset();
-
- // CHECK LOWER BOUND: Is byteOffset < extent begin?
- // If so, we are doing a load/store
- // before the first valid offset in the memory region.
+ // At this point we know that rawOffset is not default-constructed so we may
+ // call this:
+ NonLoc ByteOffset = rawOffset.getByteOffset();
----------------
steakhal wrote:
> I don't think we need an explanation here.
> BTW This "optional-like" `RegionRawOffsetV2` makes me angry. It should be an
> `std::optional<RegionRawOffsetV2>` in the first place.
> You don't need to take actions, I'm just ranting...
Yes, I was also very annoyed by this "intrustive optional" behavior and I
seriously considered refactoring the code to use a real std::optional, but I
didn't want to snowball this change into a full-scale rewrite so I ended up
with the assert in the constructor and this comment. Perhaps I'll refactor it
in a separate NFC...
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:173
+ const MemSpaceRegion *SR = rawOffset.getRegion()->getMemorySpace();
+ if (SR->getKind() != MemRegion::UnknownSpaceRegionKind) {
+ // a pointer to UnknownSpaceRegionKind may point to the middle of
----------------
steakhal wrote:
>
You're completely right, I just blindly copied this test from the needlessly
overcomplicated `computeExtentBegin()`.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:212-215
}
- assert(state_withinUpperBound);
- state = state_withinUpperBound;
+ if (state_withinUpperBound)
+ state = state_withinUpperBound;
----------------
steakhal wrote:
> You just left the guarded block in the previous line.
> By moving this statement there you could spare the `if`.
Nice catch :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148355/new/
https://reviews.llvm.org/D148355
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
