https://issues.dlang.org/show_bug.cgi?id=16508

Stanislav Blinov <stanislav.blinov@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |stanislav.blinov@gmail.com

--- Comment #1 from Stanislav Blinov <stanislav.blinov@gmail.com> ---
These are actually two separate issues. One is for Phobos: it would seem that classInstanceAlignment doesn't even look at the base. That one has nothing to do with druntime.

Second is allocating on a (ludicrous) alignment requirement. If you're still around and willing to split 'em up yourself, please do so.

--

https://issues.dlang.org/show_bug.cgi?id=16508

Max Samukha <maxsamukha@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |maxsamukha@gmail.com

--- Comment #2 from Max Samukha <maxsamukha@gmail.com> ---
std.traits.classInstanceAlignment is broken because it uses "alignof" to calculate the maximum alignment of the class fields, and "alignof" on class/struct fields is broken because it returns the alignment of field types instead of the alignment of fields. __traits(classInstanceSize) is broken because it returns a useless value that doesn't account for the trailing padding required for proper alignment of object arrays. GC allocations are also broken as they return misaligned class objects.

--

https://issues.dlang.org/show_bug.cgi?id=16508

kinke <kinke@gmx.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kinke@gmx.net

--- Comment #3 from kinke <kinke@gmx.net> ---
> __traits(classInstanceSize) is broken because it returns a useless value that doesn't account for the trailing padding required for proper alignment of object arrays.

There's no such thing as class instance arrays, only arrays of class references, so that's a non-issue.

--

https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #4 from Max Samukha <maxsamukha@gmail.com> ---
(In reply to kinke from comment #3)

> 
> There's no such thing as class instance arrays, only arrays of class references, so that's a non-issue.

I'm curious why you chose to nitpick - it's unlikely you didn't understand what I meant.

Class instance types exist implicitly and are commonly represented by structs. The return type of std.typecons.scoped is an example, as you know. It is reasonable to expect arrays (and, in general, sequentially stored objects) of such structs to be usable. __traits(classInstanceSize) and std.trait.classInstanceAlignment are used almost exclusively to calculate the size/alignment for such structs. Neither produces results usable for that purpose.

--

https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #5 from kinke <kinke@gmx.net> ---
(In reply to Max Samukha from comment #4)
> I'm curious why you chose to nitpick - it's unlikely you didn't understand what I meant.

Just stumbled upon this issue and wanted to correct this - `classInstanceSize` doesn't need a fix.

> Class instance types exist implicitly and are commonly represented by structs.  The return type of std.typecons.scoped is an example, as you know. It is reasonable to expect arrays (and, in general, sequentially stored objects) of such structs to be usable.

A field like `align(C.alignof) void[__traits(classInstanceSize, C)] buffer` should make sure there's appropriate padding, solely based on the align declaration, which is required anyway and apparently does need fixing.

--

https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #6 from Max Samukha <maxsamukha@gmail.com> ---
(In reply to kinke from comment #5)
> (In reply to Max Samukha from comment #4)
> > I'm curious why you chose to nitpick - it's unlikely you didn't understand what I meant.
> 
> Just stumbled upon this issue and wanted to correct this - `classInstanceSize` doesn't need a fix.
> 
> > Class instance types exist implicitly and are commonly represented by structs.  The return type of std.typecons.scoped is an example, as you know. It is reasonable to expect arrays (and, in general, sequentially stored objects) of such structs to be usable.
> 
> A field like `align(C.alignof) void[__traits(classInstanceSize, C)] buffer` should make sure there's appropriate padding, solely based on the align declaration, which is required anyway and apparently does need fixing.

(I assume you meant align(classInstanceAlignment!C), which currently produces
wrong results due to this bug)

I'm sorry to disagree. __traits(classInstanceSize) should return the stride of an array of class instances, similarly to what .sizeof does for structs.

--

https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #7 from kinke <kinke@gmx.net> ---
(In reply to Max Samukha from comment #6)
> (I assume you meant align(classInstanceAlignment!C), which currently
> produces wrong results due to this bug)

Yeah sorry, of course. We'll probably need a __traits(classInstanceAlignment) I
guess, as `align(64) class C { int x; }` won't be inferrable by looking at the
field alignments.

> I'm sorry to disagree.

No problem there. But it's the size the compiler uses (for the init symbol, the
TypeInfo.initializer() etc.), not some library bug, and it's working.

--

https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #8 from Max Samukha <maxsamukha@gmail.com> ---
(In reply to kinke from comment #7)
> (In reply to Max Samukha from comment #6)
> > (I assume you meant align(classInstanceAlignment!C), which currently
> > produces wrong results due to this bug)
> 
> Yeah sorry, of course. We'll probably need a
> __traits(classInstanceAlignment) I guess, as `align(64) class C { int x; }`
> won't be inferrable by looking at the field alignments.

Yeah, I forgot about the external align. It seems the trait is necessary.

> 
> > I'm sorry to disagree.
> 
> No problem there. But it's the size the compiler uses (for the init symbol,
> the TypeInfo.initializer() etc.), not some library bug, and it's working.

Bugs will most likely be revealed in those when/if the align attribute is fixed and people start using it.

--

https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #9 from kinke <kinke@gmx.net> ---
(In reply to Max Samukha from comment #8)
> > No problem there. But it's the size the compiler uses (for the init symbol,
> > the TypeInfo.initializer() etc.), not some library bug, and it's working.
> 
> Bugs will most likely be revealed in those when/if the align attribute is fixed and people start using it.

Nope - again, class instances are never stored consecutively. Only wrapper structs can be, and as long as they align the backing buffer as required, all is/will be working fine.

--

https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #10 from Max Samukha <maxsamukha@gmail.com> ---
(In reply to kinke from comment #9)
> (In reply to Max Samukha from comment #8)
> > > No problem there. But it's the size the compiler uses (for the init symbol,
> > > the TypeInfo.initializer() etc.), not some library bug, and it's working.
> > 
> > Bugs will most likely be revealed in those when/if the align attribute is fixed and people start using it.
> 
> Nope - again, class instances are never stored consecutively. Only wrapper structs can be, and as long as they align the backing buffer as required, all is/will be working fine.

By that argument, a struct object is likewise an ephemeral entity with a backing buffer of bytes, and therefore 'sizeof' of a struct should not include the padding. We'd better stop at that to avoid a philosophical debate. A template returning the desired result will be easy to define after the bugs are fixed.

--