[haiku-commits] Re: haiku: hrev52133 - src/apps/mediaconverter

  • From: Stephan Aßmus <superstippi@xxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Thu, 26 Jul 2018 23:25:50 +0200

Hi,

Am 26.07.2018 um 20:32 schrieb Julian Harnath:

Am 26.07.2018 um 12:13 schrieb Stephan Aßmus <superstippi@xxxxxx>:
Am 26.07.2018 um 11:43 schrieb Julian Harnath:
Am 26.07.2018 um 11:26 schrieb Stephan Aßmus <superstippi@xxxxxx 
<mailto:superstippi@xxxxxx>>:
You did /not/ say media_format ist not a POD type.
Relevant article from C++ FAQ:
https://isocpp.org/wiki/faq/intrinsic-types#pod-types
Given that, current media_format is not a POD (has private members, an overload 
of assignment op, etc).

The point is that memset() is currently effective at initializing a media_format including the 
complex fields that it has, since all of those end up in POD fields themselves, and the enum values 
which mean "uninitalized" all have a value of "0". It has been designed that 
way.

While that is true for the individual members, as pointed out above, the 
resulting struct still does not meet the requirements for POD in C++03, and so, 
I don’t think that doing memset/memcpy/etc on it is safe. Unfortunately, I 
cannot find references for this in C++03, so I’ll have to stay with a „don’t 
think“ here…
I don’t oppose having a Clear() method, but I think that the current 
implementation is bad, because it most probably uses undefined behaviour 
(although it might just work in practice, but that’s not enough). gcc has its 
reason to throw a warning, and moving the memset to Clear() only hides that by 
adding a level of indirection.

Btw for C++11, documentation is easier to find and the situation is more clear 
[0]: memset/memcpy/etc are only allowed [1] on „trivially copyable“ [2] types.

Thanks for the info. Isn't a "private:" declaration just a compile time thing? If so, that would leave "overload of assignment" as a potential reason for why media_format may not be "trivially copyable". I don't really see how, though, i.e. why that would affect object storage. But I probably just don't know enough about C++ implementation details. Or is there more hiding in that "etc"?

There’s another thing which I’ll just pull out of another message…

I begin to think you may be confused as to how unions work. There isn't a concept of an 
"active field". The memory of the fields overlaps, and you can access the same 
memory via all the defined fields at the same time.

That is actually not true in C++. Unions have been used that way („type 
punning“, i.e. write into the union using one of its types, then read with 
another) for a long time, but even in C it was not originally allowed. Some 
compilers (e.g. gcc) implemented a compiler-specific extension to allow it [3]. 
Since C99, it actually is allowed in the C language [4], but still not in C++ 
[5].
And yes, that makes unions way less useful in C++ (and in older C versions) 
than many people think... it’s really all just about saving memory, not easy 
access via multiple types.

Again, thanks for all the info, I've learned a lot. I guess that's what I get for being mostly self-taught. IIRC, I even learned about unions mostly from seeing how media_format works. What I don't understand is how code in one place can even know which union member has been initialized in another place of the code. It would seem to make it necessary to always have fields in unions wich carry this information (such as media_format::type). But I don't mean for you to keep digging, I should do that myself.

In any case, in my previous mails, I think more than once, I was asking the honest question if memset() may have unwanted side-effects, or be ineffective at fully initializing the union. A simple "Yes, please google it" would have been fine for me. And in fact I did google it, but only found stuff that confirmed what I learned before. (In essence, that media_format should be "trivially copyable".) Alter I saw that media_format also has the meta_data stuff, which makes it not trivially copyable, but that was taken into account where memset() was used, although not always in the correct way.

Best regards,
-Stephan





Other related posts: