|
Description
Simon McVittie
2011-05-31 08:24:18 UTC
Created attachment 47383 [details] [review] [PATCH 01/14] Add some missing symbols to the docs Created attachment 47384 [details] [review] [PATCH 02/14] Mention which header to include in the gtk-doc Created attachment 47385 [details] [review] [PATCH 03/14] Remove stray empty subsection Created attachment 47386 [details] [review] [PATCH 04/14] Ignore non-API source files correctly gtk-doc only cares about the basename of the files. Created attachment 47387 [details] [review] [PATCH 05/14] Clean up docs for DBusGConnection * introductory doc-comment for the (pseudo-)class * attach the doc-comment for the GType to the macro, not the internal function * rename function parameters to match what the header file says Created attachment 47388 [details] [review] [PATCH 06/14] Tidy up docs for DBusGMessage * document the instance struct * make argument names match the headers * document the GType macro, not the function it calls, and mention it in the documentation (since this is a boxed type, so it's non-obvious) Created attachment 47389 [details] [review] [PATCH 07/14] Tidy up docs for DBusGError and its pseudo-methods * attach the introductory doc-comments to the enum and macro names, not the internal function * remove docs for method parameters which didn't actually exist * more cross-references * make the domain macro visible in the documentation index Created attachment 47390 [details] [review] [PATCH 08/14] Add DBUS_TYPE_CONNECTION, DBUS_TYPE_MESSAGE to the docs, and document them better Created attachment 47391 [details] [review] [PATCH 09/14] Remove declaration of dbus_pending_call_get_g_type, which doesn't exist Created attachment 47392 [details] [review] [PATCH 10/14] specialized types: improve documentation and document more things Created attachment 47393 [details] [review] [PATCH 11/14] DBusGProxy: mark struct contents as private Created attachment 47394 [details] [review] [PATCH 12/14] DBusGProxy: make argument names in declaration, definition and docs consistent Created attachment 47395 [details] [review] [PATCH 13/14] DBusGProxy: misc documentation tidying Created attachment 47396 [details] [review] [PATCH 14/14] DBusGProxy: be more pedantic about boolean returns Returning false doesn't strictly imply that the error is set; if you're being pedantic enough, it's really a tri-state where TRUE indicates success, FALSE indicates failure, and calling the function incorrectly results in undefined behaviour (currently a critical warning and returning FALSE). Bug-NB: NB#218973 Cosimo, could you review these please? They resolve NB#218973. I've rebased the branch in cgit onto dbus-glib 0.96, which I released yesterday. Review of attachment 47383 [details] [review]: Fine to me Review of attachment 47384 [details] [review]: Fine to me. Review of attachment 47385 [details] [review]: Also fine. Review of attachment 47386 [details] [review]: OK Review of attachment 47387 [details] [review]: Fine Review of attachment 47388 [details] [review]: Also Fine Review of attachment 47389 [details] [review]: Fine for me either with or without the "iff" amend. ::: dbus/dbus-glib.c @@ +187,3 @@ * the given name. This function is intended to be invoked on a + * #GError returned from an invocation of a remote method, e.g. via + * dbus_g_proxy_end_call(). It will silently return FALSE for errors %FALSE @@ +193,2 @@ * * Returns: TRUE iff the remote error has the given name %TRUE Also, since you're touching the file, what do you think of extending "iff" as "if and only if", for clarity sake? One might thing it's a typo, which wouldn't change much, but it's still documentation :) I don't know if it occurs elsewhere. Review of attachment 47390 [details] [review]: Fine for me Review of attachment 47391 [details] [review]: OK Review of attachment 47392 [details] [review]: ::: dbus/dbus-gtype-specialized.c @@ +822,1 @@ * I find confusing "If <return value> otherwise it's an error". I expect that "otherwise" in this sentence structure has other possible return values, while it is actually warning about something stronger. What about something like (rephrase as you wish, not native speaker here :) -- It requires the collection having elements of fixed size (i.e. ...). Otherwise, it is an error to call this function. [what can it happen?] Return the contents of the array. -- @@ +822,3 @@ * + * It's probably a bad idea to call this. + * I'd move this sentence in the beginning of the description, to help users to drop the idea of using it before actually imaging an actual use for it :) Probably joint together with the "it's an error" case above. Review of attachment 47393 [details] [review]: K. Review of attachment 47394 [details] [review]: OK Review of attachment 47395 [details] [review]: Fine Review of attachment 47396 [details] [review]: OK (In reply to comment #22) > ::: dbus/dbus-glib.c > @@ +187,3 @@ > * the given name. This function is intended to be invoked on a > + * #GError returned from an invocation of a remote method, e.g. via > + * dbus_g_proxy_end_call(). It will silently return FALSE for errors > > %FALSE Good catch, I'll annotate that > Also, since you're touching the file, what do you think of extending "iff" as > "if and only if", for clarity sake? Yeah, we should be kind to non-mathematicians :-) > I don't know if it occurs elsewhere. I'll grep. (In reply to comment #25) > What about something like (rephrase as you wish, not native speaker here :) > > -- > It requires the collection having elements of fixed size (i.e. ...). > Otherwise, it is an error to call this function. [what can it happen?] Yeah, you're right. I'll look for a good phrasing. > + * It's probably a bad idea to call this. > + * > > I'd move this sentence in the beginning of the description, to help users to > drop the idea of using it before actually imaging an actual use for it :) Yeah, perhaps that would be better :-) Created attachment 51673 [details] [review] [1/6 v2] Tidy up docs for DBusGError and its pseudo-methods * attach the introductory doc-comments to the enum and macro names, not the internal function * remove docs for method parameters which didn't actually exist * more cross-references * make the domain macro visible in the documentation index --- Fixed according to Cosimo's comments regarding % and iff Created attachment 51674 [details] [review] [2/6] dbus-gtype-specialized: warn if vtables have missing callbacks It's just about conceivable that they wouldn't segfault, if application authors carefully avoided the unimplemented functionality... but still. --- I noticed while fixing the documentation that most functions dereference struct members without checking. That's fine, but we should check it *somewhere*. Created attachment 51676 [details] [review] [3/6] dbus_g_type_collection_get_fixed: check preconditions on the type and vtable Previously, if it wasn't a collection or didn't have the fixed_accessor, we'd just segfault. Not ideal. --- Also noticed while fixing Cosimo's review comments. Created attachment 51677 [details] [review] [4/6] dbus_g_type_collection_value_iterate, etc.: check that the type is suitable Otherwise we'd probably crash when we cast the vtable to an inappropriate type, and call its methods with inappropriate arguments as a result. --- Like Attachment #51676 [details] but for the other functions, which call callbacks whose existence is checked in Attachment #51674 [details]. Created attachment 51679 [details] [review] [5/6] specialized types: improve documentation and document more things --- Many changes since Attachment #47392 [details], please re-review. Created attachment 51680 [details] [review] [6/6] Fix various minor documentation bugs I applied the patches that Cosimo approved. With these new 6 patches applied, we have almost 100% documentation coverage. The only missing thing now is documentation for the members of DBusGError. Re-reviewed and they look all ok :) Thanks, fixed in git for 0.98. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.