NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:125
+ Msg = std::string(llvm::formatv(
+ "Possibly not enough {0} bytes for array allocation which "
+ "requires "
----------------
f00kat wrote:
> martong wrote:
> > Maybe the below could be a wording that's more easy to follow?
> > `{0} bytes is possibly not enough for array allocation which ...`
> Yeah. Sure. I have some troubles with english :)
Why do we say "possibly"? Where does the uncertainty come from?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:189
+ if (const MemRegion *MRegion = PlaceVal.getAsRegion()) {
+ if (const ElementRegion *TheElementRegion =
+ MRegion->getAs<ElementRegion>()) {
----------------
martong wrote:
> f00kat wrote:
> > f00kat wrote:
> > > NoQ wrote:
> > > > NoQ wrote:
> > > > > The sequence of `FieldRegion`s and `ElementRegion`s on top of a base
> > > > > region may be arbitrary: `var.a[0].b[1][2].c.d[3]` etc.
> > > > >
> > > > > I'd rather unwrap those regions one-by-one in a loop and look at the
> > > > > alignment of each layer.
> > > > Alternatively, just decompose the whole region into base region and
> > > > offset and see if base region has the necessary alignment and the
> > > > offset is divisible by the necessary alignment.
> > > > The sequence of FieldRegions and ElementRegions on top of a base region
> > > > may be arbitrary: var.a[0].b[1][2].c.d[3] etc.
> > >
> > > But i think(hope) I already do this and even have tests for this cases.
> > > For example
> > > ```void test22() {
> > > struct alignas(alignof(short)) Z {
> > > char p;
> > > char c[10];
> > > };
> > >
> > > struct Y {
> > > char p;
> > > Z b[10];
> > > };
> > >
> > > struct X {
> > > Y a[10];
> > > } Xi; // expected-note {{'Xi' initialized here}}
> > >
> > > // ok. 2(X align) + 1 (offset Y.p) + 1(align Z.p to 'short') + 1(offset
> > > Z.p) + 3(index)
> > > ::new (&Xi.a[0].b[0].c[3]) long;
> > > }```
> > >
> > > Cases with multidimensional arrays will also be handled correctly because
> > > method 'TheElementRegion->getAsArrayOffset()' calculates the offset for
> > > multidimensional arrays
> > > ```void testXX() {
> > > struct Y {
> > > char p;
> > > char b[10][10];
> > > };
> > >
> > > struct X {
> > > Y a[10];
> > > } Xi;
> > >
> > > ::new (&Xi.a[0].b[0][0]) long;
> > > }```
> > >
> > > I can explain the code below for ElementRegion if needed.
> > ?
> Yeah, the tests are convincing and I think that you are handling the regions
> well.
>
> On the other hand, this code is getting really complex, we should make it
> easier to read and understand.
> E.g. `FieldOffsetValue` should be explained more, is it the offset started
> from the the start address of the multidimensional array, or it is just the
> offset from one element's start address?
> Also, you have two variables named as `Offset`. They are offsets from which
> starting address? Perhaps we should have in the comments a running example,
> maybe for `&Xi.a[0].b[0][0]`? I mean is `FieldOffseValue` is standing for `b`
> or for `a`?
> Alternatively, just decompose the whole region into base region and offset
> and see if base region has the necessary alignment and the offset is
> divisible by the necessary alignment.
I expect this to be, like, 5 lines of code. I don't understand why the current
code is so complicated, it looks like you're considering multiple cases but
ultimately doing the same thing.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76996/new/
https://reviews.llvm.org/D76996
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
