Bug 65755 - [PATCH] two fixes for api doc
Summary: [PATCH] two fixes for api doc
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: Other All
: medium normal
Assignee: Havoc Pennington
QA Contact:
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-06-14 14:03 UTC by Chengwei Yang
Modified: 2013-08-23 08:24 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH 1/2] Doc-fix-invalid-usage-of-doxygen-param-command (3.85 KB, patch)
2013-06-14 14:05 UTC, Chengwei Yang
Details | Splinter Review
[PATCH 2/2] Doc: fix incorrect param names, missing params (13.80 KB, patch)
2013-06-14 14:06 UTC, Chengwei Yang
Details | Splinter Review
[PATCH v2 1/2] Doc: fix invalid usage of doxygen @param command (3.86 KB, patch)
2013-06-15 00:43 UTC, Chengwei Yang
Details | Splinter Review
[PATCH v2 2/2] Doc: fix incorrect param names, missing params, non-exist params (14.12 KB, patch)
2013-06-15 00:44 UTC, Chengwei Yang
Details | Splinter Review
[PATCH v3 1/2] Doc: fix invalid usage of doxygen @param command (3.86 KB, patch)
2013-06-15 07:49 UTC, Chengwei Yang
Details | Splinter Review
[PATCH v3 2/2] Doc: fix incorrect param names, missing params, non-exist params (14.10 KB, patch)
2013-06-15 07:50 UTC, Chengwei Yang
Details | Splinter Review
[PATCH v4] Doc: fix incorrect param names, missing params, non-exist params (14.18 KB, patch)
2013-06-20 14:06 UTC, Chengwei Yang
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chengwei Yang 2013-06-14 14:03:06 UTC
There are a lot of doxygen warning throw out by $make -C doc. The two patches try to fix some of them, especially for incorrect @param, missing params, non-exists params. These patches didn't document any new one.
Comment 1 Chengwei Yang 2013-06-14 14:05:21 UTC
Created attachment 80810 [details] [review]
[PATCH 1/2] Doc-fix-invalid-usage-of-doxygen-param-command
Comment 2 Chengwei Yang 2013-06-14 14:06:06 UTC
Created attachment 80811 [details] [review]
[PATCH 2/2] Doc: fix incorrect param names, missing params
Comment 3 Simon McVittie 2013-06-14 14:15:06 UTC
Comment on attachment 80810 [details] [review]
[PATCH 1/2] Doc-fix-invalid-usage-of-doxygen-param-command

Review of attachment 80810 [details] [review]:
-----------------------------------------------------------------

Mostly looks good...

::: dbus/dbus-sysdeps-util-unix.c
@@ +381,4 @@
>  
>  /**
>   * Attempt to ensure that the current process can open
> + * at least limit file descriptors.

Does doxygen have markup for "this is the name of a parameter", or failing that, "typeset this in monospace"? (In gtk-doc you'd use @limit).

I feel as though limit should be made to stand out from the text around it somehow, to make it clear that we're talking about the parameter of that name: if there isn't any conventional markup, maybe it could be emphasized or put in single quotes or something?
Comment 4 Simon McVittie 2013-06-14 14:22:30 UTC
Comment on attachment 80811 [details] [review]
[PATCH 2/2] Doc: fix incorrect param names, missing params

Review of attachment 80811 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-connection.c
@@ +5206,4 @@
>   *
>   * @param connection the connection
>   * @param data return location for audit data 
> + * @param data_size return length of audit data

Either "return location for length of audit data" (using "return location" as a noun), or "used to return length of audit data" (using "return" as a verb)

::: dbus/dbus-keyring.c
@@ +700,4 @@
>   * of the given user credentials. If the credentials are #NULL or
>   * empty, uses those of the current process.
>   *
> + * @param credentials username to get keyring for, or #NULL

Is this actually a username now, or something else?

::: dbus/dbus-message.c
@@ +4721,1 @@
>   * @param len the length of str

"the length of buf" with some sort of emphasis/markup (see other patch) on buf, please

::: dbus/dbus-object-tree.c
@@ +741,4 @@
>   *
>   * @param tree the global object tree
>   * @param message the message to dispatch
> + * @param found_object return found object

"return location for the object" or something?

::: dbus/dbus-sysdeps-unix.c
@@ +3571,4 @@
>  
>  /**
>   * quries launchd for a specific env var which holds the socket path.
> + * @param socket_path return the socket path in DBusString

"used to return" or something?

How is it returned? Is the previous content of the DBusString thrown away, or does this function append to it?

::: dbus/dbus-sysdeps-util-unix.c
@@ +449,3 @@
>   *
>   * @param severity a severity value
>   * @param msg a printf-style format string

"a printf-style format string, followed by its arguments"? Or is there a way to document the "..." in Doxygen?

(In gtk-doc you'd use "@...: the arguments for @msg")

::: dbus/dbus-sysdeps-win.c
@@ +412,4 @@
>   * on exec. Should be called for all file
>   * descriptors in D-Bus code.
>   *
> + * @param handle the file descriptor

On Windows there are several file-like things: it might be a C runtime library file descriptor, it might be a Winsock socket file descriptor, or it might be a HANDLE. Which is this?

::: dbus/dbus-test.c
@@ +94,4 @@
>   * (with --enable-tests=no)
>   *
>   * @param test_data_dir the directory with test data (test/data normally)
> + * @param specific_test specific test

I'm fairly sure this is "... or #NULL to run all tests".
Comment 5 Chengwei Yang 2013-06-15 00:43:34 UTC
Created attachment 80831 [details] [review]
[PATCH v2 1/2] Doc: fix invalid usage of doxygen @param command
Comment 6 Chengwei Yang 2013-06-15 00:44:11 UTC
Created attachment 80832 [details] [review]
[PATCH v2 2/2] Doc: fix incorrect param names, missing params,  non-exist params
Comment 7 Chengwei Yang 2013-06-15 00:46:12 UTC
(In reply to comment #3)
> Comment on attachment 80810 [details] [review] [review]
> [PATCH 1/2] Doc-fix-invalid-usage-of-doxygen-param-command
> 
> Review of attachment 80810 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Mostly looks good...
> 
> ::: dbus/dbus-sysdeps-util-unix.c
> @@ +381,4 @@
> >  
> >  /**
> >   * Attempt to ensure that the current process can open
> > + * at least limit file descriptors.
> 
> Does doxygen have markup for "this is the name of a parameter", or failing
> that, "typeset this in monospace"? (In gtk-doc you'd use @limit).
> 
> I feel as though limit should be made to stand out from the text around it
> somehow, to make it clear that we're talking about the parameter of that
> name: if there isn't any conventional markup, maybe it could be emphasized
> or put in single quotes or something?

Done, "@a" used to put parameter into italic style.
Comment 8 Chengwei Yang 2013-06-15 00:48:16 UTC
(In reply to comment #4)
> Comment on attachment 80811 [details] [review] [review]
> [PATCH 2/2] Doc: fix incorrect param names, missing params
> 
> Review of attachment 80811 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: dbus/dbus-connection.c
> @@ +5206,4 @@
> >   *
> >   * @param connection the connection
> >   * @param data return location for audit data 
> > + * @param data_size return length of audit data
> 
> Either "return location for length of audit data" (using "return location"
> as a noun), or "used to return length of audit data" (using "return" as a
> verb)
> 
> ::: dbus/dbus-keyring.c
> @@ +700,4 @@
> >   * of the given user credentials. If the credentials are #NULL or
> >   * empty, uses those of the current process.
> >   *
> > + * @param credentials username to get keyring for, or #NULL
> 
> Is this actually a username now, or something else?
> 
> ::: dbus/dbus-message.c
> @@ +4721,1 @@
> >   * @param len the length of str
> 
> "the length of buf" with some sort of emphasis/markup (see other patch) on
> buf, please
> 
> ::: dbus/dbus-object-tree.c
> @@ +741,4 @@
> >   *
> >   * @param tree the global object tree
> >   * @param message the message to dispatch
> > + * @param found_object return found object
> 
> "return location for the object" or something?
> 
> ::: dbus/dbus-sysdeps-unix.c
> @@ +3571,4 @@
> >  
> >  /**
> >   * quries launchd for a specific env var which holds the socket path.
> > + * @param socket_path return the socket path in DBusString
> 
> "used to return" or something?
> 
> How is it returned? Is the previous content of the DBusString thrown away,
> or does this function append to it?
> 
> ::: dbus/dbus-sysdeps-util-unix.c
> @@ +449,3 @@
> >   *
> >   * @param severity a severity value
> >   * @param msg a printf-style format string
> 
> "a printf-style format string, followed by its arguments"? Or is there a way
> to document the "..." in Doxygen?
> 
> (In gtk-doc you'd use "@...: the arguments for @msg")
> 

Seems doxygen smart enough to handle "...", see the generated doc like

Parameters:
    severity	a severity value
    msg	        a printf-style format string
    args	arguments for the format string

All the others done in PATCH v2.

> ::: dbus/dbus-sysdeps-win.c
> @@ +412,4 @@
> >   * on exec. Should be called for all file
> >   * descriptors in D-Bus code.
> >   *
> > + * @param handle the file descriptor
> 
> On Windows there are several file-like things: it might be a C runtime
> library file descriptor, it might be a Winsock socket file descriptor, or it
> might be a HANDLE. Which is this?
> 
> ::: dbus/dbus-test.c
> @@ +94,4 @@
> >   * (with --enable-tests=no)
> >   *
> >   * @param test_data_dir the directory with test data (test/data normally)
> > + * @param specific_test specific test
> 
> I'm fairly sure this is "... or #NULL to run all tests".
Comment 9 Chengwei Yang 2013-06-15 07:49:40 UTC
Created attachment 80838 [details] [review]
[PATCH v3 1/2] Doc: fix invalid usage of doxygen @param command
Comment 10 Chengwei Yang 2013-06-15 07:50:13 UTC
Created attachment 80839 [details] [review]
[PATCH v3 2/2] Doc: fix incorrect param names, missing params,  non-exist params
Comment 11 Chengwei Yang 2013-06-15 07:53:07 UTC
(In reply to comment #7)
> (In reply to comment #3)
> > Comment on attachment 80810 [details] [review] [review] [review]
> > [PATCH 1/2] Doc-fix-invalid-usage-of-doxygen-param-command
> > 
> > Review of attachment 80810 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > Mostly looks good...
> > 
> > ::: dbus/dbus-sysdeps-util-unix.c
> > @@ +381,4 @@
> > >  
> > >  /**
> > >   * Attempt to ensure that the current process can open
> > > + * at least limit file descriptors.
> > 
> > Does doxygen have markup for "this is the name of a parameter", or failing
> > that, "typeset this in monospace"? (In gtk-doc you'd use @limit).
> > 
> > I feel as though limit should be made to stand out from the text around it
> > somehow, to make it clear that we're talking about the parameter of that
> > name: if there isn't any conventional markup, maybe it could be emphasized
> > or put in single quotes or something?
> 
> Done, "@a" used to put parameter into italic style.

change in patch v3.

use "@p" instead of "@a" since it already used in some where else. Cut from doxygen doc

\p <word>

Displays the parameter <word> using a typewriter font. You can use this command to refer to member function parameters in the running text.
Comment 12 Chengwei Yang 2013-06-20 09:54:32 UTC
Simon, any comment about the new version of patches?
Comment 13 Simon McVittie 2013-06-20 12:04:41 UTC
Comment on attachment 80838 [details] [review]
[PATCH v3 1/2] Doc: fix invalid usage of doxygen @param command

Review of attachment 80838 [details] [review]:
-----------------------------------------------------------------

r+
Comment 14 Simon McVittie 2013-06-20 12:13:24 UTC
Comment on attachment 80839 [details] [review]
[PATCH v3 2/2] Doc: fix incorrect param names, missing params,  non-exist params

Review of attachment 80839 [details] [review]:
-----------------------------------------------------------------

I have a limited amount of time to work on D-Bus, so I try to spend most of it on things that are higher-priority than the Doxygen, like fixing bugs/reviewing bugfixes. I'll review this stuff eventually, but please be patient.

::: dbus/dbus-connection.c
@@ +5205,4 @@
>   * connection.
>   *
>   * @param connection the connection
> + * @param data return location for length of audit data

No, "return location for audit data" was correct here...

@@ +5205,5 @@
>   * connection.
>   *
>   * @param connection the connection
> + * @param data return location for length of audit data
> + * @param data_size return length of audit data

... *this* is the "return location for length of audit data".

::: dbus/dbus-keyring.c
@@ +697,4 @@
>  
>  /**
>   * Creates a new keyring that lives in the ~/.dbus-keyrings directory
> + * of the given user @p credentials. If the @p credentials are #NULL or

"the user represented by @p credentials" maybe?

@@ +701,3 @@
>   * empty, uses those of the current process.
>   *
> + * @param credentials the user credentials for, or #NULL

"the user credentials for" isn't English :-)

Perhaps something like: "a set of credentials representing a user"?

::: dbus/dbus-sysdeps-unix.c
@@ +3571,4 @@
>  
>  /**
>   * quries launchd for a specific env var which holds the socket path.
> + * @param socket_path append found socket path to

"append the socket path to this string" or "string to which the socket path will be appended", perhaps?

::: dbus/dbus-sysdeps-win.c
@@ +412,4 @@
>   * on exec. Should be called for all file
>   * descriptors in D-Bus code.
>   *
> + * @param handle the socket file descriptor

Are you sure this is actually a file descriptor, and not a Win32 HANDLE? (Likewise for all the Windows socket API.)
Comment 15 Simon McVittie 2013-06-20 12:15:00 UTC
Comment on attachment 80838 [details] [review]
[PATCH v3 1/2] Doc: fix invalid usage of doxygen @param command

I applied this one.
Comment 16 Chengwei Yang 2013-06-20 14:06:28 UTC
Created attachment 81112 [details] [review]
[PATCH v4] Doc: fix incorrect param names, missing params, non-exist  params


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.