Status

()

Core
Build Config
--
minor
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Presently, when one runs |make documentation| in the doxygen code, doxygen complains that the configuration file has some deprecated options in it and needs to update. This is the first part of doxygen.cfg.in that I will update.

There are some other minor changes that I'm making, enumerated below:
* Defaulting HAVE_DOT to YES. The dot diagrams are nice to see.
* Modifying EXCLUDE_PREFIXES to clean up the list a lot (like adding nsI so that nsIAbCard is filed under `A' and not `I').
* Letting doxygen comments apply to all in a group. This feature is moderately used in mailnews code at least.

I still need to play around with letting the user decide how to set up EXCLUDE_PREFIXES, but that is currently seeming like too much trouble for so little value.

I am also trying to decide how best to let doxygen handle the undocumented stuff (my current sense is to have it include it as well for completeness's sake).

Comments/questions/concerns/thoughts?
(Assignee)

Comment 1

9 years ago
Created attachment 321414 [details] [diff] [review]
Updated doxygen.cfg.in

Fixed up the in. Basically, I ran doxygen -u doxygen.cfg.in and tweaked a few parameters for nicer output:
* Modified the IGNORE_PREFIX to better represent the list (basically distribute the interfaces as well)
* A few dot parameters to generate the graphs and keep them somewhat sane
* Add all classes to the list, independent of their documentation level
* Turned off warning for no documentation. That really spams the output
Attachment #321414 - Flags: superreview?
Attachment #321414 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #321414 - Flags: superreview?(benjamin)
Attachment #321414 - Flags: superreview?
Attachment #321414 - Flags: review?(cbiesinger)
Attachment #321414 - Flags: review?
Comment on attachment 321414 [details] [diff] [review]
Updated doxygen.cfg.in

-GENERATE_TODOLIST      = YES
+GENERATE_TODOLIST      = NO

why?
Attachment #321414 - Flags: review?(cbiesinger) → review+
Attachment #321414 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Comment 3

9 years ago
(In reply to comment #2)
> (From update of attachment 321414 [details] [diff] [review])
> -GENERATE_TODOLIST      = YES
> +GENERATE_TODOLIST      = NO
> 
> why?
> 

(Sorry for the delay)

I changed this because a) no one seemed to use it, and b) I figured that people would be using bugzilla instead for the todo list. If you prefer, I'll change this back to the original setting.
XForms uses it...
(Assignee)

Comment 5

9 years ago
I did not know that; since that is the only problem you have, I will remove it from the final patch before checking in...
(Assignee)

Comment 6

9 years ago
I've been looking in finer details at doxygen, and have come up with the following points to raise:

* If/when the doxygen parser is fixed to handle xpidl, there is a problem in that nsISupports becomes documented as well, which really impacts dot graphs. See <http://doxygen.db48x.net/mozilla/html/interfacensIMsgFolder__coll__graph.svg> for an example.
* I've not found a way to deal with the, e.g., nsIntSize should be sorting under I but, e.g., nsIObserver should sort under O. Sounds worthwhile enough to feature request, though.
* Macro handling can be better defined, I think. I need to run tests for macro handling.
* The EXCLUDE_PATTERNS looks fragile; some other option may be needed to fix this better.

More, dependent on documentation standards:
* /defgroup, /ingroup (or @ if you prefer) look like they'd be helpful for making documentation better; at that point, adding GROUP_GRAPHS may be helpful.
* Probably nice to document more file headers, but that would probably involve adding a separate documentation command (alldocs, uberdocs?)

I'll hold off on a new patch until I get macros/nsISupports figured out (I've changed the GENERATE_TODOLIST already).
(In reply to comment #6)
> * If/when the doxygen parser is fixed to handle xpidl, there is a problem in
> that nsISupports becomes documented as well, which really impacts dot graphs.
> See
> <http://doxygen.db48x.net/mozilla/html/interfacensIMsgFolder__coll__graph.svg>
> for an example.

Yea, it's pretty cool. We could filter nsISupports out of the graphs pretty easily, I think.

> * I've not found a way to deal with the, e.g., nsIntSize should be sorting
> under I but, e.g., nsIObserver should sort under O. Sounds worthwhile enough to
> feature request, though.

I'm not sure there's any way to get this right 100% of the time, short of writing full AI that can read and understand English.

> More, dependent on documentation standards:
> * /defgroup, /ingroup (or @ if you prefer) look like they'd be helpful for
> making documentation better; at that point, adding GROUP_GRAPHS may be helpful.

Yea, making groups for all of the modules would make the docs so much cleaner. We also need to fix the bidi module so that it doesn't use \mainpage.

> * Probably nice to document more file headers, but that would probably involve
> adding a separate documentation command (alldocs, uberdocs?)

Heh, I just run doxygen over the source tree instead of the objdir. See http://doxygen.db48x.net/mozilla-full/html/

Comment 8

9 years ago
(In reply to comment #7)
> > * I've not found a way to deal with the, e.g., nsIntSize should be sorting
> > under I but, e.g., nsIObserver should sort under O. Sounds worthwhile enough to
> > feature request, though.
> 
> I'm not sure there's any way to get this right 100% of the time, short of
> writing full AI that can read and understand English.

What sort of cases would a heuristic like "strip 'ns' prefix, plus leading 'I' if followed by another capital letter" get wrong?
(In reply to comment #8)
> What sort of cases would a heuristic like "strip 'ns' prefix, plus leading 'I'
> if followed by another capital letter" get wrong?

nsIDNService.cpp
(In reply to comment #9)
> (In reply to comment #8)
> > What sort of cases would a heuristic like "strip 'ns' prefix, plus leading 'I'
> > if followed by another capital letter" get wrong?
> 
> nsIDNService.cpp
> 

and nsIOService, and so on.
(Assignee)

Comment 11

9 years ago
Created attachment 325604 [details] [diff] [review]
Latest patch

Re-requesting r/sr because I've updated significantly from last patch.

Specifically, I've excluded nsCOMPtr_base and nsISupports from documentation because their presence makes the graphs look horrible.

I also enabled preprocessing, which makes it take longer, but, OTOH, makes the documentation more reliable.

This should probably go on both mozilla-central and CVS trunk.
Attachment #321414 - Attachment is obsolete: true
Attachment #325604 - Flags: superreview?
Attachment #325604 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #325604 - Flags: superreview?(benjamin)
Attachment #325604 - Flags: superreview?
Attachment #325604 - Flags: review?(cbiesinger)
Attachment #325604 - Flags: review?
Comment on attachment 325604 [details] [diff] [review]
Latest patch

I have concerns about the preprocessing: does it use the actual preprocessed sources used by mozilla, or does it preprocess independently? We should ideally either explicitly make .i files or use -save-temps: many defines are set by the makefiles themselves and those wouldn't make it into some doxygen-particular preprocessing step.
It uses doxygen's built-in preprocessor, to process the #defines/#ifdefs in the input files. The config file can also specify a list of defines.

As for .i files, I think we only want to process .idl/.h files, which won't work so well with that...
Comment on attachment 325604 [details] [diff] [review]
Latest patch

Whatever, though I still don't understand why running doxygen on compiler preprocessor output doesn't make sense.
Attachment #325604 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Comment 15

9 years ago
A few points:
1. This patch, although based off of the 1.9.0.* sources, will land on 1.9.* trunk if r/sr'd.
2. While doing some tests locally, I discovered that doxygen's preprocessor is dumb and doesn't look through the directories recursively. Including dist/include/xpcom and dist/include/nspr are probably sufficient, however, to trickle down nscore.h and prtypes.h to all files (doxygen will also include any files in the current directory). However, changing this segment would require changing configure.in...
3. One useful thing that using doxygen's preprocessor does is allow us to document #define's.
Attachment #325604 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 16

9 years ago
Committed to m-c (with slight change that nsISupports removed from EXCLUDE_SYMBOLS, per biesi's comments over irc):

pushing to ssh://hg.mozilla.org/mozilla-central/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
changeset:   16140:616403a5a73e
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.