Created attachment 106029 [details] [review]
Patch declaring GstTagList as "typedef struct _GstTabList GstTagList"

I _think_ that should be ok, but could you elaborate on why it's needed?

It would also be great if you cuold create patches in future using 'cvs diff -u -p ' (see http://gstreamer.freedesktop.org/wiki/SubmittingPatches ).

Thanks for your attention.  I will definitely pay attention to appropriately submitting patches when I do.  I'll elaborate below as to why I'm requesting this change:

To generate the C++ API, we run source files (*.hg and *.ccg) files through a glibmm script called gmmproc which then generates the *.h and *.cc files for the wrapper library.  In this case, the source files TagList.hg and TagList.ccg are processed to produce TagList.h and TagList.cc.

Presently, gmmproc assumes that structs are declared in the header files as "typedef struct _ExampleStructure ExampleStructure" and thus in this case it inserts the following lines at the top of the generated header files:

#ifndef DOXYGEN_SHOULD_SKIP_THIS
extern "C" { typedef struct _GstTagList GstTagList; }
#endif

This produces a type mismatch compilation error.  gmmproc has been pre-written and it is really a fine helper and I don't see how it can be changed to work on a case by case basis so I thought that this change might be easy to do for gstreamer.

To get a sense of what I'm explaining please see the Unable to predeclare structs in the Problems in C API section (http://www.gtkmm.org/docs/gtkmm-2.4/docs/tutorial/html/aphs06.html) of Appendix H (http://www.gtkmm.org/docs/gtkmm-2.4/docs/tutorial/html/aph.html) of the Programming with gtkmm online book (http://www.gtkmm.org/docs/gtkmm-2.4/docs/tutorial/html/index.html).

If this is acceptable, it would be a great help in this gstreamermm wrapping process of your fine software.  Many C++ programmers will benefit and be thankful. :-)

With your permission, I'm also attaching a more appropriate patch.  I hope I've answered your questions.

Created attachment 106036 [details] [review]
Replacement patch

Created attachment 108987 [details] [review]
alternative patch

Does this also work for you?

(In reply to comment #5)
> Created an attachment (id=108987) [edit]
> alternative patch
> 
> Does this also work for you?
> 

Nearly, but I'm getting a minor compile error.  Let me show you what our generated include file looks like and maybe we can find some workaround.  This is the top of the generated include file:

...
#include <gst/gsttaglist.h>

...
extern "C" { typedef struct _GstTagList GstTagList; }
...

And this is the compile error:

In file included from taglist.cc:4:
../../gstreamer/gstreamermm/taglist.h:34: error: using typedef-name '_GstTagList' after 'struct'
/opt/usr-jhbuild/include/gstreamer-0.10/gst/gsttaglist.h:143: error: '_GstTagList' has a previous declaration here

Thanks for all your help.

Ok, any chance you could come up with a patch that changes the header file only and doesn't require any additional casts elsewhere? (otherwise we'd be breaking API in a way, which isn't really nice)

(In reply to comment #7)
> Ok, any chance you could come up with a patch that changes the header file only
> and doesn't require any additional casts elsewhere? (otherwise we'd be breaking
> API in a way, which isn't really nice)
> 

I'll try.  Thanks.

(In reply to comment #8)
> I'll try.  Thanks.
> 

I found a solution which in fact doesn't require any changes on your part and for this I must apologize.  As I mentioned, we run a script called gmmproc (part of glibmm) on *.hg and *.ccg files to produce the C++ *.h and *.c files.  To declare the C++ types we use "class macros" in the *.hg files that "describe" how to use the C API to declare the C++ types.  I was using the following to declare the C++ type Gst::TagList:

class TagList
{
  _CLASS_BOXEDTYPE(TagList, GstTagList, gst_tag_list_new, gst_tag_list_copy, gst_tag_list_free)

public:
...

The _CLASS_BOXEDTYPE macro takes (in order) a C++ type, the corresponding C type, the function used to create a new object and the function to free the object once it is created -- see the _CLASS_BOXEDTYPE docs in the Class macros section of "The .hg and .ccg files (http://www.gtkmm.org/docs/gtkmm-2.4/docs/tutorial/html/sec-wrapping-hg-files.html) of the gtkmm wrapping docs (http://www.gtkmm.org/docs/gtkmm-2.4/docs/tutorial/html/aph.html).

I changed the C type from "GstTagList" to "GstStructure" like so:

class TagList
{
  _CLASS_BOXEDTYPE(TagList, GstStructure, gst_tag_list_new, gst_tag_list_copy, gst_tag_list_free)

public:
...

and compilation works fine.  Again, I should have understood that a GstTagList is also a GstStructure and could have avoided an unnecessary bug report.  It is fixed here so I'll close bug reports related to this one, but thanks for your kind efforts to assist.  I'll try to be more careful.

No worries, glad you got it working.

It seems I spoke too soon.  I don't want to re-open this request, but at this point, I think I may have no choice.  The fix I described above was a fix but only a temporary one because now I'm getting errors related to the duplicity of functions  (methods) which gmmproc generates to wrap each C type of the corresponding C++ wrapper type.

As I just described, gmmproc generates a method (called a "wrap" method) for the C type given in the "class macros" of the C++ type.  In this case, one wrap method is generated for Gst::Structure and another for Gst::TagList.  But, because the C type for Gst::TagList is the same as that of Gst::Structure (GstStructure), another wrap method is generated for the Gst::Structure C type which ambiguates the orignal Gst::Structure wrap method.  Here's the build error I get:

In file included from wrap_init.cc:45:
taglist.h:251: error: new declaration 'Gst::TagList Glib::wrap(GstStructure*, bool, bool)'
../../gstreamer/gstreamermm/structure.h:372: error: ambiguates old declaration 'Gst::Structure Glib::wrap(GstStructure*, bool, bool)'
make[5]: *** [wrap_init.lo] Error 1

I'm left with my original request hoping that the change may not break API in any significant way.  It it does, we will hold off wrapping GstTagList for now.

Before I finish, however, would the following idea as to why API may not be broken be valid?  As the patch is now, the casts occur in gst_tag_list_add_valist and gst_tag_list_add_valist_values but only when calling gst_tag_list_add_value_internal (which requires a GstStructure).  Since gst_tag_list_add_value_internal is "not exposed", users would still have to use the exposed GstTagList API for working with GstTagLists and none of the exposed GstTagList API has changed.  Is this right?

Again, I really appreciate your attention to our efforts.  I'd also accept consultation with others to see what they think.  Thanks again.

The code in gsttaglist.c that needs changing is internal code, that's true, but there may be plugins in other gstreamer modules, external plugins or applications that contain code which assumes that GstTagList and GstStructure can be used interchangeably.

I think fixing your parser would be the best option if you can't come up with something that maintains full API compatibility.

Okay.  I'll see what I can do.(In reply to comment #12)
> The code in gsttaglist.c that needs changing is internal code, that's true, but
> there may be plugins in other gstreamer modules, external plugins or
> applications that contain code which assumes that GstTagList and GstStructure
> can be used interchangeably.
> 

I see.

> I think fixing your parser would be the best option if you can't come up with
> something that maintains full API compatibility.
> 

Okay.  I'll see what can be done.  Thanks.

One final comment on this: The ambiguity I described above occur precisely because the interchangeability of GstTagList and GstStructure.  Finding a solution for this I think will be pretty difficult. 

Okay, I was able to find a solution that allows wrapping of GstTagList for now.  I implemented a custom class macro like the one used for GstStructure but with an extra parameter to the wrap method so that the ambiguities between "Gst::TagList Glib::wrap(GstStructure*)" and "Gst::Structure Glib::wrap(GstStructure*)" disappear.

I might not have explained that the reason for the ambiguities is that GstTagList is declared with "typedef GstStructure GstTagList" which in essence makes it a GstStructure and since C++ is strongly typed, it takes GstTagList to in fact be a GstStructure thus causing the ambiguity above.  This makes it impossible to use the same class macro for GstTagList as for GstStructure.

Further, I had to have gmmproc generate the TagList.h and TagList.c files from the TagList.hg and TagList.ccg (the actual source files we edit and work with to wrap the API) and remove the top section from the *.h file where GstTagList is declared as "typedef struct _GstTagList GstTagList".  The only way this top section would not be a problem is if GstTagList is actually declared as "typedef struct _GstTagList GstTagList" in the C API.

I can no longer rely on the build process for the *.h and *.c files to be automatically generated.  To make changes, I must re-edit the *.hg and *.ccg files, add these files again as dependencies in the build process, run the build process to regenerate the *.h and *.c files, remove the "typedef struct _GstTagList ..." from the top of the *.h file and remove the *.hg and *.ccg from the build dependency once again to avoid problems for end users.

As a closing question:  Would the following excerpt from the "How to define and implement a new GObject" section (http://library.gnome.org/devel/gobject/unstable/howto-gobject.html) of the GObject reference where "typedef struct ...." is used to declare types apply to GstTagList (I'm still trying to fully understand the GType/GObject programming guidelines)?  If they don't I'll be perfectly happy to continue to use the above described method to continue wrapping GstTagList; if they do maybe this can be something considered for future?  Thanks:


...

 The basic conventions for any header which exposes a GType are described in the section called “Conventions”. Most GObject-based code also obeys one of of the following conventions: pick one and stick to it.

    * If you want to declare a type named bar with prefix maman, name the type instance MamanBar and its class MamanBarClass (name is case-sensitive). It is customary to declare them with code similar to the following:

      /*
       * Copyright/Licensing information.
       */

      #ifndef MAMAN_BAR_H
      #define MAMAN_BAR_H

      /*
       * Potentially, include other headers on which this header depends.
       */

      /*
       * Type macros.
       */

      typedef struct _MamanBar MamanBar;
      typedef struct _MamanBarClass MamanBarClass;

      struct _MamanBar {
        GObject parent;
        /* instance members */
      };

      struct _MamanBarClass {
        GObjectClass parent;
        /* class members */
      };

      /* used by MAMAN_TYPE_BAR */
      GType maman_bar_get_type (void);

      /*
       * Method definitions.
       */

      #endif

> As a closing question:  Would the following excerpt from the "How to define
> and implement a new GObject" section (...) of the GObject reference where
> "typedef struct ...." is used to declare types apply to GstTagList (...)?
> If they don't I'll be perfectly happy to continue to use the
> above described method to continue wrapping GstTagList; if they do maybe this
> can be something considered for future?

I believe the original reason for the first-typedef-then-declare-the-structure was to please gtk-doc's header parser, which seems to be (or have been) similarly limited to the parser you're using.

It doesn't really matter whether these guidelines "apply" or not (and FWIW: GstStructures are not GObjects, but generally we try to do things the way described in the manual too).

The headers are like they are now, and while it may be an inconvenience to you, I don't really see us breaking API in the current stable series just for that.

We'll be able to change this in GStreamer-0.11 though, ie. next time we'll break API/ABI.

I've added a FIXME in the header so we don't forget about this when doing 0.11. Closing this bug now...

commit c8da2b18796295022694333e1967c8d94218a5e5
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Thu Sep 10 10:05:38 2009 +0200

    taglist: Add FIXME for 0.11 to not typedef GstTagList to be a GstStructure
    
    See bug #518934.