Created attachment 28222 [details] [review]
add_const_to_tag_list_foreach.diff

I think this patch is ok to apply to the stable branch because it won't break
any programs that currently work.

It's theoretically unsafe because it casts the GstTagList* argument to a
non-const GstStructure* to pass it to gst_structure_field_foreach(). But it's
actually ok because gst_structure_field_foreach() doesn't modify the structure.

Created attachment 28223 [details] [review]
add_gst_structure_field_foreach_const.diff

This patch includes the last one.

It adds a gst_structure_field_foreach_const() function that takes a const
GstStructure* and calls a GstStructureForeachConstFunc. I added a new function
rather than change the old one because that would break some previously-working
programs. On the other hand, it's probably incorrect to modify the structure
inside a foreach...

Anyway, this patch is only tangentially related to the previous one, so if you
want, I can make a new bug for it.

I think we a greed to not duplicate functions because of constness issues but
just use const and add in the docs a warning that you must not do some specific
things if your structure/tag list really is a const. There are other examples
(like gst_caps_get_structure) where this is already practiced.

The only thing I'm not sure about is if the tag list in GstTagListForEachFunc
really should be a const...

I'd like to get David's input on how to handle these constness issues before
changing anything though.

See the comment on gst_caps_get_structure().  The goal is to avoid making the
application do casts from a const pointer to a non-const pointer.

I don't mind having separate gst_blah() and gst_blah_const() functions, and with
a little creative programming, these can be aliased to the same code.  (In fact,
I consider such aliasing a requisite for doing it correctly.)

However, during a stable series, it's not wise to be doing such changes.  Just
because we _can_ add symbols or make backward-compatible changes doesn't mean we
_should_, since that means that applications need to depend on 0.8.2, instead of
just 0.8.0.  Arguably, they have to do this already, but I'd like to not make
the problem worse.

We should solve this like it is with structures right now:

gboolean gst_structure_foreach (const GstStructure      *structure,
				GstStructureForeachFunc  func,
                                gpointer user_data);
gboolean gst_structure_map_in_place (GstStructure *structure,
                                     GstStructureMapFunc func,
                                     gpointer user_data);

The MapFunc takes a non-const GValue*, and the ForeachFunc takes a const GValue.
The current tag_list_for_each should then be renamed to map_in_place.
Milestoning to 0.9.x.

Well it turns out my statement was rather silly. The foreach function just
passes the names of the tags, not their values. The tag list api is very unlike
other parts of gstreamer it seems. So I am just going to change the prototype to
have const, because the foreach function doesn't even have a pointer to the
value, and entries shouldn't be added or removed in the foreach.