232032 - XPIDL generated headers may give false impression an interface is frozen

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

I was looking at the generated header nsISelectionController.h (which is NOT
frozen) and saw the following contained within:

/**
 * Interface for manipulating and querying the current selected range
 * of nodes within the document.
 *
 * @status FROZEN
 * @version 1.0
 */

This is caused by the forward declaration of nsISelection (the next line in the
generated output is:

class nsISelection; /* forward declaration */

This is a problem IMO because it is one of the first things a reader sees when
the file is opened.  In fact, this totally threw me because at the time I opened
the file, I could only see 35 lines without scrolling.  As it happens, the
forward declaration is on line 36 of the generated output in this case, so I did
not realize for a while that this was referring to nsISelection instead of
nsISelectionController, (which was the interface I cared about since I was
reading it).

I fear that people may make this same mistake and falsely assume that an
interface is frozen when it is not.  Additionally, someone wishing to grep their
generated headers for @status FROZEN would get unreliable results.  I am filing
this as an xpidl bug, because I don't think that XPIDL should output comments
describing forward declared interfaces into other interface headers.  Especially
since many of our javadoc style comments do not actually name what they are
describing such as

/**
 * nsIFoo
 *
 * An interface for ....
 */

although we probably want to start doing this as well.

For a more generic argument (since the presence or lack thereof of @status
comments is not really something xpidl needs to concern itself with really), a
forward declare denotes that the actual semantics of the class are not really
important to know for the present and the definition will come later.  So
comments describing the forward declared interface are unnecessary for a reader
to understand the generated header.  If they care to figure out what a forward
declared interface does, they are quite free to look it up in its own header.

Another argument is that it is without a doubt important to understand the base
interface of a derived interface, but those interfaces are always #include'd by
law, and #included files do not get comments output into the generated header. 
So in this scenario, the more important interface will not receive a comment,
but the not-so-important interface will.  That hardly seems fair ;-)

For reference, this behavior was introduced as part of mccabe's fix for bug
24734 (rev 1.75 of xpidl_header.c).  I have a patch which I believe should fix this.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch

Comment 2

[David Bradley]

I just wonder why they went to the trouble to do this? I agree with your
argument, but I wonder if there's something we're missing. Caillon, if shaver
doesn't respond get at least brendan or someone, jband/jst, to comment and buy
in, and it will be r= for me. Hey, probably reduce the build times slightly ;-)

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 139793 [details] [diff] [review]
Patch

Totally the right thing.  Thanks for the excellent analysis, too.

BTW: I tend to be much better at keeping up with my request-queue mail than
random bugmail (of which I still a fair bit), if you need me for other patches
in the future.

sr=shaver

Comment 4

[David Bradley]

Comment on attachment 139793 [details] [diff] [review]
Patch

r=dbradley

Go for it.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Oops, I had thought I requested reviews but it doesn't appear so.  Thanks for
the quick responses, both of you!

Fix checked in 01/24/2004 15:01 PST.