GNOME Bugzilla – Bug 609946
Misguided implicit copy in Gio::MemoryInputStream::add_data()
Last modified: 2014-02-12 14:43:59 UTC
On looking into bug #609552, I noticed that two of the three overloaded add_data() methods of Gio::MemoryInputStream implicitly create a copy of the data before returning. According to a comment in the code, this is meant as a convenience feature to free the caller from the burden to keep the memory around for the lifetime of the stream. However, I question the appropriateness of doing so, for the following reasons: 1) It introduces new semantics that are inconsistent with the C API. Deviations from the C API should be properly justified, and we should make sure we have understood the intent of the original API before doing so. 2) I understand a memory input stream to be a somewhat low-level utility which enables the direct use of a block of memory as a data source. If all you need is to pass along data, not caring about intermediate copies, streams seem to be a somewhat clumsy interface. 3) I don't think it's useful to read from main memory asynchronously. Thus, managing the life time of the input data is not actually a concern in what I consider to be the intended usage pattern: create a temporary memory input stream, tell it where the data is, use the stream as input, and destroy the stream. Asynchronous reads could perhaps be useful for mmap'ed blocks of a file on disk, but if you made an effort to use mmap() to reduce copies, you very likely decidedly don't want an implicit copy to happen in the stream interface. Apart from the implicit copy, I also think the std::string overload should go, to emphasize the raw nature of the API. And the destroy function -- for those cases where you do need it -- should be a sigc::slot<void,void*> instead of the GDestroyFunc plain function pointer.
yes, you're right. This API is quite poor and was done without adequate thought due to lack of time (and lack of review, I guess). Also, some of it (e.g. the destroy function) was simply an oversight. I had wrapped the plain C types to get things working quickly and planned to come back and fix things properly later, but then apparently forgot...
So what should be done?
Daniel?
Please respond.
daniel, do you have any sugestions for how to achieve the sigc::slot destroy function? Usually we do this by passing the slot as user_data and then invoking the slot in a proxy callback. But a GDestroyNotify has no user_data parameter, so I'm not sure how to do it without resorting to some really ugly hacks. I guess this is why the plain GDestroyNotify argument is here in the first place...
Created attachment 159305 [details] [review] Deprecate misguided MemeoryInputStream::add_data() overloads (bug #609946)
It looks like a bug in the C API then.
Ah. I already noticed this apparently, and filed bug #589887 against the C API.
Created attachment 268396 [details] [review] patch: Gio::MemoryInputStream: Add add_data() taking a sigc::slot Colin Walters mentions in bug 589887 comment 23 that our problem with the missing user_data parameter in g_memory_input_stream_new_from_data() and g_memory_input_stream_add_data() was in a way solved when g_memory_input_stream_new_from_bytes() and g_memory_input_stream_add_bytes() were added to GMemoryInputStream in glib 2.34. We can use g_memory_input_stream_add_bytes() in Gio::MemoryInputStream::add_data(). g_bytes_new_with_free_func() has the necessary user_data parameter. If my patch is acceptable, bug 589887 can be closed. No more change is needed in glib.
Created attachment 268397 [details] Test case
Yes, thanks, that seems good, particularly with the test case. Can we add a test case to the others that are run with "make check"?
I have pushed the patch in comment 9, and a patch that adds the test case glibmm/tests/giomm_memoryinputstream. I have also closed the glib bug 589887.