martong added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91 + SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State); + const auto SizeOfTargetCI = SizeOfTarget.getAs<nonloc::ConcreteInt>(); + if (!SizeOfTargetCI) ---------------- xazax.hun wrote: > Here, instead of getting `SizeOfTarget` and `SizeOfPlace` as `ConcreteInt`s, > I think you should rather use `evalBinOp` to compare them. That method is > more future proof as if we cannot constraint these values down to a single > integer but we still have some information about them a sufficiently smart > solver could prove the relationship between the symbolic values. I am not sure if `evalBinOp` is that useful here, because we need the concrete values anyway when we issue the diagnostics. We'd like to present the concrete sizes in bytes. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:32 + const MemRegion *BaseRegion = MRegion->getBaseRegion(); + assert(BaseRegion == Offset.getRegion()); + ---------------- NoQ wrote: > martong wrote: > > This assertion fails on real code quite often (e.g. on LLVM/Clang code). I > > don't really understand why. @NoQ what is you understanding on this? > > Perhaps I could try to reduce a case from the real code to see an example. > `RegionOffset` cannot really represent symbolic offsets :( You should be able > to re-add the assertion after checking for symbolic offsets. > > And, yeah, you can always easily reduce any crash with `creduce`. I think protecting with `if (Offset.hasSymbolicOffset())` is just fine, instead of the assertion. ================ Comment at: clang/test/Analysis/placement-new.cpp:6 +// RUN: -analyzer-output=text -verify \ +// RUN: -triple x86_64-unknown-linux-gnu + ---------------- NoQ wrote: > Wow, somebody actually remembers to add a target triple for tests that depend > on the target triple! My respect, sir. Thanks! :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71612/new/ https://reviews.llvm.org/D71612 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
