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.
Created attachment 80810 [details] [review] [PATCH 1/2] Doc-fix-invalid-usage-of-doxygen-param-command
Created attachment 80811 [details] [review] [PATCH 2/2] Doc: fix incorrect param names, missing params
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 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".
Created attachment 80831 [details] [review] [PATCH v2 1/2] Doc: fix invalid usage of doxygen @param command
Created attachment 80832 [details] [review] [PATCH v2 2/2] Doc: fix incorrect param names, missing params, non-exist params
(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.
(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".
Created attachment 80838 [details] [review] [PATCH v3 1/2] Doc: fix invalid usage of doxygen @param command
Created attachment 80839 [details] [review] [PATCH v3 2/2] Doc: fix incorrect param names, missing params, non-exist params
(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.
Simon, any comment about the new version of patches?
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 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 on attachment 80838 [details] [review] [PATCH v3 1/2] Doc: fix invalid usage of doxygen @param command I applied this one.
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.