GNOME Bugzilla – Bug 652746
Documentation of plain boxed structs broken
Last modified: 2011-09-08 16:07:29 UTC
Declaring a simple struct in the header file as follows typedef struct { gdouble r; gdouble g; gdouble b; gdouble a; } GwyRGBA; and documenting it /** * GwyRGBA: * @r: The red component. * @g: The green component. * @b: The blue component. * @a: The alpha (opacity) value. * * RGBA colour specification type. **/ does not work if the type is registered as boxed (i.e. it has a get-type, copy and free functions). This is a regression, it used to work in 1.15. First, the declaration gets converted to typedef struct _GwyRGBA GwyRGBA; in the documentation, which beside being an utter lie (there is *no* struct _GwyRGBA anywhere and nor it should be) hides all the members and one gets warnings about unused documentation ../../libgwy/rgba.c:245: warning: Field description for GwyRGBA::a is not used from source code comment block. ../../libgwy/rgba.c:245: warning: Field description for GwyRGBA::r is not used from source code comment block. ../../libgwy/rgba.c:245: warning: Field description for GwyRGBA::b is not used from source code comment block. ../../libgwy/rgba.c:245: warning: Field description for GwyRGBA::g is not used from source code comment block. instead of getting them documented. The reason is probably similar as in bug 652732, i.e. the type gets into the hierarchy file because it's boxed and has a get-type function, so CheckIsObject() thinks that it is an object (even though it is not) and starts doing all sorts of evil things to the symbol.
Yes, adding a /* < public > */ can be used as an intermediate 'fix'. I'll have to see how I can make CheckIsObject() smarter. To be honest I don't like the different policies applied to objects and non objects anyway. It would have been much better to have a consistent scheme - like everything is public (c'mon it is C anyway) unless someone puts a /* < private > */. Unfortunately Damon won't say much about what he was thinking here back in the years.
I could make a patch to make CheckIsObject() smarter in the sense it would (a) only take things derived from GObject or GInterface as objects, which should be possible because the hierarchy is known, or (b) at least ignore things derived from GFlags, GEnum or GBoxed. I am not sure if CheckIsObject() is already the core of the problem (it is just as far I was able to track it to something definite) or it actually originates elsewhere.
The core problem is elsewhere. The HEAD version of gtk-doc seems to have some logic in outputting enums and structs that prevents bug 652732 but on the other hand causes this. It goes as follows: If it has a registered GType (i.e. CheckIsObject() returns true) it is treated specially. For both it means appending -struct or -enum to the ID[*], resolving ID conflict with the <anchor>. But tied to this is making the struct members invisible and treating it completely as an object struct - which is obviously unwanted because plain-old-data structs can have public fields and be boxed at the same time. So first, why are the <anchors> generated at all? There must be some reason for that but the only things I get them generated for are enums and boxed types - which is a pure annoyance. Second, I can mark all my plain old data /*< public >*/, but I think this would be *wrong* exactly because it is plain old data, not objects. So, why is the default to hide the members?
Created attachment 190106 [details] [review] find object roots This patch finds the root of each `object' in the hierarchy, it to %ObjectRoots. <anchor>s in the summary are then added only for non-enum, non-flag and non-boxed types. Which presumably means objects and interfaces if one has more of them in one section. It does not actually fix the problem yet but once %ObjectRoots is available it can be used easily in similar manner in OutputStruct() to check whether the symbol really is an object.
Also, I hate git. Another hour of my life completely wasted and at the end the only way I was able to wrestle my changes from it after `git commit' was using `git show THE-UGLY-COMMIT-ID'.
Thanks for string to solve this, I will look at it during the weekend. I just switched countried and jobs and thus am a bit busy. What was the probelm with git. I hated it at times too, but now like it very much. Just let me know if I can help.
Stefan, I am trying to make a more complete patch but while I can make it work as I think it should work I am afraid some things were done on purpose, e.g. now http://developer.gnome.org/gtk3/stable/GtkTreeModel.html#GtkTreePath takes you to the top of the page while to get to the actual struct documentation you need to go to http://developer.gnome.org/gtk3/stable/GtkTreeModel.html#GtkTreePath-struct Some real work was put into making it this so complicated. So, why? Note that in case of GtkTreePath you will not learn anything useful about it at `the actual struct documentation' because it is opaque. However, the same happens in case public plain old data or enums if they have a GType registered -- which seems to be a random characteristics, not a distinguishing feature to me.
Created attachment 190196 [details] [review] proposed patch New patch. - %ObjectRoots holds the root parent of each type - CheckIsObject() is rewritten in terms of %ObjectRoots (should be faster) - CheckIsObject() returns false for enums and flags now - CheckIsObject() returns true for boxed types, this is not changed - however, boxed types that document at least one member default to public members So, as in C++ - class had private members by default - struct has public members by default If you want a struct with all private members just do not document them. If you want mixed public/private then mark them explicitly.
(In reply to comment #7) > Stefan, I am trying to make a more complete patch but while I can make it work > as I think it should work I am afraid some things were done on purpose, e.g. > now > > http://developer.gnome.org/gtk3/stable/GtkTreeModel.html#GtkTreePath > > takes you to the top of the page while to get to the actual struct > documentation you need to go to > > http://developer.gnome.org/gtk3/stable/GtkTreeModel.html#GtkTreePath-struct > > Some real work was put into making it this so complicated. So, why? > > Note that in case of GtkTreePath you will not learn anything useful about it at > `the actual struct documentation' because it is opaque. However, the same > happens in case public plain old data or enums if they have a GType registered > -- which seems to be a random characteristics, not a distinguishing feature to > me. I totally agree that is is undesired and annoying. It only makes sense for gobjects as they are the 'reason' for a separate documentation page. I'll apply your patch in the coming days and make some before after comparison. A fix for that issue would make a worthy 1.18 release.
(In reply to comment #8) > If you want a struct with all private members just do not document them. Please, don't do this. How am I to detect undocumented POD structs then?
(In reply to comment #10) > (In reply to comment #8) > > If you want a struct with all private members just do not document them. > > Please, don't do this. How am I to detect undocumented POD structs then? /* * MySecretObject: * @field1: bla * */ I mean just use /* */ instead of /** */
I mean. If I add a new public struct to my headers and forget to document it, what would happen? If you assume it's private, I wouldn't get -undocumented warnings, right? I don't like that.
(In reply to comment #10) > (In reply to comment #8) > > If you want a struct with all private members just do not document them. > > Please, don't do this. How am I to detect undocumented POD structs then? I just exended the part # If no parameters are filled in, we don't generate the description # table, for backwards compatibility. to apply here too because I tried to avoid creating a policy. Apparently some people want structs public others want them private. I definitely do not want to mark all POD structs /*< public >*/ because if they are POD with visible memebers they are _meant_ to be public. But understandably, others have stuff in their headers they consider implementation details and do not want to mark all structs /*< private >*/. Does another --default-FOO mkdb option make sense?
(In reply to comment #13) > (In reply to comment #10) > > (In reply to comment #8) > > > If you want a struct with all private members just do not document them. > > > > Please, don't do this. How am I to detect undocumented POD structs then? > > I just exended the part > > # If no parameters are filled in, we don't generate the description > # table, for backwards compatibility. > > to apply here too because I tried to avoid creating a policy. Apparently some > people want structs public others want them private. I definitely do not want > to mark all POD structs /*< public >*/ because if they are POD with visible > memebers they are _meant_ to be public. Right. > But understandably, others have stuff > in their headers they consider implementation details and do not want to mark > all structs /*< private >*/. I guess we just disagree on the "understandably" part :). I'd say force them to add /*< private >*/. It's the right thing to do for their users too.
Created attachment 190426 [details] [review] proposed patch New version: makes `objects' of type GBoxed also unconditionally public (i.e. plain old data).
Review of attachment 190426 [details] [review]: The patch looks good. Would be nice to add a test case to the gobject suite to quickly try it. Witch the current patch the visibility policy gets a bit cluttered. Right now we see to have GObject Instance : private GObject class : public GBoxed : public GEnum, GFlags : public POD : private Maybe it would indded be a good idea to have flags for gtkdoc-mkdb, e.g. --def-{instance,class,boxed,enum,flags,struct}-scope={public,private} ::: gtkdoc-mkdb.in @@ +5380,3 @@ my ($name) = @_; + my $root = $ObjectRoots{$name}; + # Let GBoxed pass as an object here. why? we don't do hat for flags/enum.
(In reply to comment #16) > POD : private I hope not. The entire thing started because registering PODs as GBoxed made them private. Unregistered POD was public and should remain public. > Maybe it would indded be a good idea to have flags for gtkdoc-mkdb, e.g. > --def-{instance,class,boxed,enum,flags,struct}-scope={public,private} Dunno. Maybe it would be best to treat everything the same (probably public) and let people mark their sources instead of guessing... > ::: gtkdoc-mkdb.in > @@ +5380,3 @@ > my ($name) = @_; > + my $root = $ObjectRoots{$name}; > + # Let GBoxed pass as an object here. > > why? we don't do hat for flags/enum. Because of id conflict. A boxed POD often has a section dedicated to it so the -struct suffix is necessary. Enums don't.
(In reply to comment #17) > (In reply to comment #16) > > POD : private > > I hope not. The entire thing started because registering PODs as GBoxed made > them private. Unregistered POD was public and should remain public. My mistake. You are right. > > > Maybe it would indded be a good idea to have flags for gtkdoc-mkdb, e.g. > > --def-{instance,class,boxed,enum,flags,struct}-scope={public,private} > > Dunno. Maybe it would be best to treat everything the same (probably public) > and let people mark their sources instead of guessing... Lets keep the patch as it is in this regard. > > > ::: gtkdoc-mkdb.in > > @@ +5380,3 @@ > > my ($name) = @_; > > + my $root = $ObjectRoots{$name}; > > + # Let GBoxed pass as an object here. > > > > why? we don't do hat for flags/enum. > > Because of id conflict. A boxed POD often has a section dedicated to it so the > -struct suffix is necessary. Enums don't. I see, could you please add a comment about that. This + the test and I'll push the patch.
Concerning the tests, is it normal that cat: ts: No such file or directory (standard_in) 2: syntax error is printed after each test subdirectory is entered?
Created attachment 195760 [details] [review] proposed patch
(In reply to comment #19) > Concerning the tests, is it normal that > > cat: ts: No such file or directory > (standard_in) 2: syntax error > > is printed after each test subdirectory is entered? I might use some bashism in the test makefiles to get some simple profiling :/ There are customized gtk-doc.make and gtk-doc.notmpl.make files under tests that take wallclock timings. There might be a race in the dependencies too :/ Are you running with MAKEFLAGS=-j<n> | n>=2 ? Regarding the scope discussion also see Bug #324880. After some more thinking for Enums, Flags, PODs the default scope=public makes sense and I don't see much reason to override that.
(In reply to comment #21) > Are you running with MAKEFLAGS=-j<n> | n>=2 ? Definitely: 3 is the smallest value I ever use (on notebook) and it was running with MAKEFLAGS=-j7 when I noticed the messages.
fixed the time stamp stuff too. ommit 0822f0d42faf1b6d9d73e12a9bb7c4158471f531 Author: Stefan Sauer <ensonic@users.sf.net> Date: Thu Sep 8 18:04:29 2011 +0200 tests: make the timestamps work with -jn