Hi,
Am 25.07.2018 um 22:16 schrieb Dario Casalinuovo:
On Wed, Jul 25, 2018 at 10:06 PM Adrien Destugues <pulkomandy@xxxxxxxxxxxxx <mailto:pulkomandy@xxxxxxxxxxxxx>> wrote:
On Wed, Jul 25, 2018 at 09:38:11PM +0200, Stephan Aßmus wrote:
> Hi,
>
> Am 25.07.2018 um 18:37 schrieb Adrien Destugues:
> > On Wed, Jul 25, 2018 at 06:06:48PM +0200, Dario Casalinuovo wrote:
> > > In any case I expect people to review correctly, in this case
it seems 3
> > > devs accepted code they didn't understand.
> >
> > Using Clear instead of memset seems fine, so that particular
patch is
> > right in the current context. How we implement Clear - or
wether it is
> > needed at all - should have been fixed earlier already.
> >
> > The previous patch where the problem lies is
> > https://review.haiku-os.org/#/c/haiku/+/215/ . I simply missed
the fact
> > that Clear() was just a memset, which of course still isn't
appropriate
> > even if hidden in a method. No one is perfect, and there were
just 2
> > devs (not 3) reviewing it.
>
> But media_format contains only POD fields, and it is a nested
union, meaning
> the same memory would be initialized more than once if done field
by field.
> All the fields are arranged in that way, that a value of "0" means
> uninitialized. Also, POD fields are not initialized in a
constructor by the
> compiler, that's why it needed to happen manually. Some part of
the patch
> re-used media_format instances in a loop and needed to reset
them, hence the
> use of memset() and now Clear(). So why, exactly, is using
memset() not
> correct? Is it because media_format /may/ be extended with
non-POD fields in
> the future? Is it because on some CPU architecture "0" may not
mean 0 for
> some data types? I don't get it. Also, the whole reasoning why
the patch is
> supposedly bad is only given now. Before it was just being called
"shit"
> with no actual reason given.
Right, the constructor is responsible for initializing things, and the
struct will be cleared to 0 only in the cases where it would have in C:
when allocating with new() or as a global. But not when used on the
stack. Hence the constructor memsetting itself.
Again media_format is non-POD.
Absolutely not! Do not memset unions, if you want to follow the C++ standard.
Reasons in the above post.
