steakhal added inline comments.
================
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();
----------------
donat.nagy wrote:
> 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...
Please :D
================
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
----------------
donat.nagy wrote:
> steakhal wrote:
> >
> You're completely right, I just blindly copied this test from the needlessly
> overcomplicated `computeExtentBegin()`.
Hold on. This would only skip the lower bounds check if it's an
`UnknownSpaceRegion`.
Shouldn't we early return instead?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:212-215
}
- assert(state_withinUpperBound);
- state = state_withinUpperBound;
+ if (state_withinUpperBound)
+ state = state_withinUpperBound;
----------------
donat.nagy wrote:
> steakhal wrote:
> > You just left the guarded block in the previous line.
> > By moving this statement there you could spare the `if`.
> Nice catch :)
On second though no. The outer if guards `state_exceedsUpperBound`.
So this check seems necessary.
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
