Last Comment Bug 912360 - [patch] NSS documentation
: [patch] NSS documentation
Status: UNCONFIRMED
:
Product: NSS
Classification: Components
Component: Documentation (show other bugs)
: trunk
: All All
: -- normal (vote)
: ---
Assigned To: nobody
:
Mentors:
: 912358 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-03 23:47 PDT by Milan Bartos
Modified: 2014-06-29 18:47 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
nss_add_doxygen.patch (90.09 KB, patch)
2013-09-03 23:47 PDT, Milan Bartos
mbartos: review? (rrelyea)
brian: feedback+
ryan.sleevi: feedback-
wtc: feedback+
Details | Diff | Review

Description Milan Bartos 2013-09-03 23:47:10 PDT
I've added doxygen documentation into NSS sources (patch attached, logo is at http://mbartos.fedorapeople.org/doxygen3/html/logo.png, I can't find option to append more attachements). There is currently no documentation and functions are not split into public/private modules. This needs to be done by NSS developers who know what that particular functions are for. There is a little README for that at http://mbartos.fedorapeople.org/README.doxygen. This patch is supposed to serve as a first step towards useful NSS documentation.

I'd like to read you opinion, ideas how to improve it etc.

Thanks,
Milan Bartos

P.S.: I've been told to ask wtc and Bob Relyea for review. I can't find Bob in that Requestee text field, so only wtc is there. Sorry
Comment 1 Milan Bartos 2013-09-03 23:47:41 PDT
Created attachment 799301 [details] [diff] [review]
nss_add_doxygen.patch
Comment 2 Milan Bartos 2013-09-03 23:51:06 PDT
*** Bug 912358 has been marked as a duplicate of this bug. ***
Comment 3 Ryan VanderMeulen [:RyanVM] 2013-09-04 10:13:14 PDT
Please don't request checkin on a patch that hasn't gotten r+ yet.
Comment 4 Wan-Teh Chang 2013-09-04 16:05:05 PDT
Comment on attachment 799301 [details] [diff] [review]
nss_add_doxygen.patch

Bob, could you review this patch? Thanks.
Comment 5 Robert Relyea 2013-09-04 16:40:37 PDT
Comment on attachment 799301 [details] [diff] [review]
nss_add_doxygen.patch

Review of attachment 799301 [details] [diff] [review]:
-----------------------------------------------------------------

yes, I can review it wtc. I asked Milan to request you or ryan just to make sure someone outside red hat sees the patch. What we really want is your feedback on whether you may have issues with the idea of the patch. Thanks,

bob
Comment 6 Wan-Teh Chang 2013-09-05 11:33:16 PDT
Comment on attachment 799301 [details] [diff] [review]
nss_add_doxygen.patch

Ryan, Brian: please take a look at this proposal to use doxygen in NSS
source files.

Bob: I am fine with using doxygen if it is fine with you.

Hi Milan,

I read your sample at http://mbartos.fedorapeople.org/README.doxygen.
The comments for functions look fine and remind me of javadoc.
The "/**<" comment delimiter for structure fields and enumerators looks
a little annoying. Can that be customized?

The \addtogroup command also looks a little annoying, but I think
it's not used that often (ideally at most twice per header file).
Comment 7 Wan-Teh Chang 2013-09-05 11:40:53 PDT
Comment on attachment 799301 [details] [diff] [review]
nss_add_doxygen.patch

Review of attachment 799301 [details] [diff] [review]:
-----------------------------------------------------------------

I took a quick look at this patch. It seems good. But I am not familiar with doxygen.
Thanks for creating this patch.

::: doc/Makefile
@@ +16,4 @@
>  name = nss-man
>  date = `date +"%Y%m%d"`
>  
> +all: prepare all-man all-html build_doxygen

Nit: we should standardize on using '-' or '_'. Existing NSS
makefiles use '_' more often.

@@ +83,5 @@
> +
> +#--------------------------------------------------------
> +# doxygen documentation
> +#--------------------------------------------------------
> +DOXYGEN_DIR = ./doxygen

Is it necessary to use "./"? I don't think things like vpath or VPATH
applies to $(DOXYGEN_DIR).

::: doc/doxygen/DoxygenLayout.xml
@@ +11,5 @@
> +    </tab>
> +    <tab type="user" visible="yes" url="globals_func.html" title="Functions"/>
> +    <tab type="classes" visible="yes" title="">
> +      <tab type="classlist" visible="yes" title="" intro=""/>
> +      <tab type="classindex" visible="$ALPHABETICAL_INDEX" title=""/> 

This doesn't really matter, but the code review tool highlights two spaces
at the end of lines in this file.
Comment 8 Ryan Sleevi 2013-09-05 14:48:30 PDT
Comment on attachment 799301 [details] [diff] [review]
nss_add_doxygen.patch

Review of attachment 799301 [details] [diff] [review]:
-----------------------------------------------------------------

I'm never sure the right flags to set, so I'm setting f-, even though I do think this is fine/good/f+ etc

My one nit is on the advice of http://mbartos.fedorapeople.org/README.doxygen in putting function documentation into the .c files.

I think it's much more useful/appropriate that they be in the header files, *not* the implementation files, on the basis that:
1) On developer machines, they should be able to look purely at the NSS headers to determine the appropriate interface
2) We should be coding against interfaces, not implementation details. If a side-effect is not documented, it's arguably a bug - OR it's an undocumented behaviour that SHOULD NOT be relied upon.

My experience with Doxygen documentation is that, while it's helpful for some degree of class diagrams, I invariably find myself resorting back to the header files to actually find how things are supposed to behave (or, in the case of NSS, the implementations as well). I'd much prefer the headers be the canonical source of the behaviour, rather than requiring a separate lookup in (the implementation, the doxygen files)

::: doc/doxygen/DoxygenLayout.xml
@@ +4,5 @@
> +  <navindex>
> +    <tab type="mainpage" visible="yes" title=""/>
> +    <tab type="pages" visible="yes" title="" intro=""/>
> +    <tab type="modules" visible="yes" title="" intro=""/>
> +    <tab type="namespaces" visible="yes" title="">

NSS, being C, won't ever have namespaces - at least in the C++ sense.

Seems like it can be dropped from the nav bar.

The same arguably applies to classes (Line 13), but ISTR that Doxygen treats structs as classes (the former we WOULD want enumerated), so it may be fine
Comment 9 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-09-05 17:01:56 PDT
(In reply to Ryan Sleevi from comment #8)
> I'm never sure the right flags to set, so I'm setting f-, even though I do
> think this is fine/good/f+ etc
> 
> My one nit is on the advice of
> http://mbartos.fedorapeople.org/README.doxygen in putting function
> documentation into the .c files.
> 
> I think it's much more useful/appropriate that they be in the header files,
> *not* the implementation files, on the basis that:
> 1) On developer machines, they should be able to look purely at the NSS
> headers to determine the appropriate interface
> 2) We should be coding against interfaces, not implementation details. If a
> side-effect is not documented, it's arguably a bug - OR it's an undocumented
> behaviour that SHOULD NOT be relied upon.

Yes, I strongly agree with rsleevi that the API documentation should be in the header files.

I am fine with using doxygen as long as:

1. It is possible to automatically verify that the doxygen stuff is correct by running the doxygen build command and looking at the result on all tier 1 platforms (Windows, Mac, and Linux). This means that there must be clear and relatively easy-to-follow instructions for getting the doxygen tools and whatever installed on all platforms.

OR:

2. We agree that it is OK to break the doxygen stuff with the expectation that somebody will fix it later.

In other words, we cannot require developers to keep the doxygen documentation generation working if it is difficult/impossible for them to test it on their platform of choice. (Note that I primarily use Windows.)
Comment 10 Robert Relyea 2013-09-05 17:04:20 PDT
Thank you ryan, brian and wtc for your feedback.

bob

Note You need to log in before you can comment on or make changes to this bug.