[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


Am 02.07.2018 um 23:14 schrieb waddlesplash:

+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)
        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,

Other related posts: