[haiku-commits] Re: haiku: hrev52055 - src/kits/media headers/os/media
- From: Stephan Aßmus <superstippi@xxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Thu, 26 Jul 2018 14:36:22 +0200
Hi,
Am 02.07.2018 um 23:14 schrieb waddlesplash:
-media_format::media_format()
+void
+media_format::Unflatten(const char *flatBuffer)
{
- memset(this, 0x00, sizeof(*this));
+ // TODO: we should not!!! make flat copies of media_format
+ memcpy(this, flatBuffer, sizeof(*this));
+ meta_data = NULL;
meta_data_area = B_BAD_VALUE;
}
Both Unflatten()...
-media_format::media_format(const media_format& other)
+void
+media_format::Clear()
{
memset(this, 0x00, sizeof(*this));
+ meta_data = NULL;
meta_data_area = B_BAD_VALUE;
+}
... and Clear() are incorrect, since they don't do anything with the
previous contents of meta_data and meta_data_area. They would actually
need to do what the destructor does, but before memset(). (And this is
actually why the manual memset() in the code that was replaced to use
Clear() was also buggy, media_format actually has a non-trivial
destructor). This is an example of producing bugs by duplicating code,
even if it is only two lines. Always refactor into methods in order to
never duplicate code. Of course, when Clear() is called from within the
constructor, and also from the copy-constructor, both meta_data and
meta_data_area would not be initialized, so the contructor and
copy-constructor need to do that /before/ calling Clear().
Note, this does not touch on the topic whether Clear() should stay
public API or should become a private method, or wether it should be
implemented via memset() or not. As is, the code is just definitely
incorrect in this regard.
Best regards,
-Stephan
Other related posts:
