On Wed, Jul 25, 2018 at 10:06 PM Adrien Destugues <pulkomandy@xxxxxxxxxxxxx>
wrote:
On Wed, Jul 25, 2018 at 09:38:11PM +0200, Stephan Aßmus wrote:
Hi,seems 3
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
factdevs 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
meaningthat 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,
the same memory would be initialized more than once if done field byfield.
All the fields are arranged in that way, that a value of "0" meansthe
uninitialized. Also, POD fields are not initialized in a constructor by
compiler, that's why it needed to happen manually. Some part of the patchthe
re-used media_format instances in a loop and needed to reset them, hence
use of memset() and now Clear(). So why, exactly, is using memset() notfields in
correct? Is it because media_format /may/ be extended with non-POD
the future? Is it because on some CPU architecture "0" may not mean 0 foris
some data types? I don't get it. Also, the whole reasoning why the patch
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.
To make it clear: I am completely open, and I am honestly asking above,
since I want to learn something I may not be aware of. I don't mean to
"suggest" anything, so please don't read anything into what I wrote. I do
however loathe the tone that has been used in this discussion.
The patch was to silence a warning from gcc. The warning may not point
to anything actually dangerous, and it is strange that it disappeared
just by moving the memset elsewhere.
We could at least memset the inner union and not the whole struct, this
way we would make sure no vtable, padding, etc is harmed.
