422555 - Add NS_OUTPARAM and NS_INOUTPARAM annotations to xpidl-generated C++ headers

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[David Mandelin [:dmandelin]]

For static checking (see bug 420933), we want GCC attributes that indicate which parameters are 'out' or 'inout' in the XPIDL-generated header files. I plan to follow the pattern used for bug 422484.

Comment 1

[David Mandelin [:dmandelin]]

A typo in the last comment: I should have said I plan to imitate bug 414063.

Comment 2

[David Mandelin [:dmandelin]]

Attachment: Patch to add in/inout param attribute annotations

This is an initial version. The patch was asked for in bug 420933 comment 12.

Comment 3

[David Mandelin [:dmandelin]]

A couple questions about this:

1. Should we put attributes for inparams as well, or let them be the default kind?

2. It would be convenient for XPCOM methods to have a function attribute marking them as XPCOM methods. (Especially if there are no attributes on inparams, because then if there are no outparam attributes, there's no way to know if it's because it's not an XPCOM method.) Could this go in NS_IMETHOD and MS_IMETHODIMP, or can those appear on non-XPCOM methods as well?

Comment 4

[Benjamin Smedberg]

1. No, I think we should just have inparam be the default, unless it comes up where we need something different

2. Well, the definition of "XPCOM Method" is somewhat nebulous... can we assume that any method returning nsresult is an XPCOM method?

Comment 5

[David Mandelin [:dmandelin]]

I was too vague about what I was getting at in #2. What I meant was, if we see a signature like

 NS_METHOD Foo(PRInt32 x, PRInt32 *aInt);

should analyses think (a) there is no attribute on aInt, so the parameter is an inparam, or (b) there are no NS_*PARAM attributes, so this wasn't output by xpidl and the analysis should try to figure it out by looking at the type (so it will conclude it's an outparam).

Comment 6

[Benjamin Smedberg]

well, the type is ambiguous... it could be either an "in [array]" or an "out"... so perhaps we either need to make a good guess, or warn that we don't have enough information to proceed.

Comment 7

[David Mandelin [:dmandelin]]

(In reply to comment #6)

Right. So, to make it possible to stop the warnings, I was hoping we could put an attribute on the function itself indicating that the parameters have been fully annotated manually/generated by xpidl.

But, on the other hand, maybe if the parameter is supposed to be an "in [array]", the type should be "const prInt32 *". So, perhaps we should have a checker (or include one in the outparam protocol checker) for types that tend to go with outparams that don't have the annotation? And then either add const or NS_OUTPARAM.

(If there is some situation where we really need an inparam with a type that looks like an outparam, we could use an NS_INPARAM for those cases only.)

Comment 8

[David Mandelin [:dmandelin]]

Attachment: Slightly revised patch

OK, I'm ready to try to land this in preparation for bug 420933. The only new thing in this revision of that patch is that it changes an apparently obsolete DEHYDRA_GCC (predating this patch) in an ifdef to STATIC_CHECKING. Btw, the positioning of the attribute is like this:

  nsresult Foo(nsIBar **aBar NSOUTPARAM);

Comment 9

[Benjamin Smedberg]

Comment on attachment 317454 [details] [diff] [review]
Slightly revised patch

Can we change the attribute names to use a NS_ prefix, to match the other annotations in nscore.h?

user("NS_scriptable")
user("NS_outparam")
user("NS_inoutparam")

r=me with that change, and a=me for landing in mozilla-central now, before leak+unit boxes are all up.

Comment 10

[David Mandelin [:dmandelin]]

Pushed.