NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:40
+ if (const auto *ERegion = dyn_cast<ElementRegion>(TVRegion)) {
+ const auto *SRegion = cast<SubRegion>(ERegion->getSuperRegion());
+ DefinedOrUnknownSVal Extent = SRegion->getExtent(svalBuilder);
----------------
xazax.hun wrote:
> Hmm, I think this logic might need some more testing. Could you add some
> tests with multi dimensional arrays?
Yeah, this code is scary because at this point literally nobody knows when
exactly do we an have element region wrapping the pointer
(https://bugs.llvm.org/show_bug.cgi?id=43364).
`MemRegion` represents a segment of memory, whereas `loc::MemRegionVal`
represents the point that is the left-hand side of that segment. I recommend
using `C.getSVal(Place).getAsRegion()` only as a reference to that point, not
the whole segment. Then you could decompose the region into a base region and
an offset (i.e., `MemRegion::getAsOffset()`), and subtract the offset from the
base region's extent to see how much space is there in the region.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:65
+ if (!State)
+ return SVal();
+ SValBuilder &svalBuilder = C.getSValBuilder();
----------------
That produces an `UndefinedVal`. I think you'd much rather have `UnknownVal`.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:115-117
+ llvm::formatv("Argument of default placement new provides storage "
+ "capacity of {0} bytes, but the allocated type "
+ "requires storage capacity of {1} bytes",
----------------
whisperity wrote:
> This message might be repeating phrases too much, and seems long. Also, I
> would expect things like //default placement new// or //argument of placement
> new// to be confusing. Not every person running Clang SA knows the
> nitty-gritty of the standard by heart...
>
> More nitpicking: even the "default" (what does this even mean, again?)
> placement new takes **two** arguments, albeit written in a weird grammar, so
> there is no "argument of" by the looks of it. I really think this is
> confusing.
>
> Something more concise, simpler, still getting the meaning across:
>
> > Storage provided to placement new is only `N` bytes, whereas allocating a
> > `T` requires `M` bytes
>
Having long messages is usually not a problem for us (instead, we'd much rather
have full sentences properly explaining what's going on), but i agree that your
text is much neater and on point.
================
Comment at: clang/test/Analysis/placement-new.cpp:6
+// RUN: -analyzer-output=text -verify \
+// RUN: -triple x86_64-unknown-linux-gnu
+
----------------
Wow, somebody actually remembers to add a target triple for tests that depend
on the target triple! My respect, sir.
================
Comment at: clang/test/Analysis/placement-new.cpp:11
+void f() {
+ short s; // expected-note-re {{'s' declared{{.*}}}}
+ long *lp = ::new (&s) long; // expected-warning{{Argument of default
placement new provides storage capacity of 2 bytes, but the allocated type
requires storage capacity of 8 bytes}} expected-note 3 {{}}
----------------
I'm legit curious what's hidden behind the regex.
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
