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

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dmandelin, Assigned: dmandelin)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
A typo in the last comment: I should have said I plan to imitate bug 414063.
(Assignee)

Comment 2

10 years ago
Created attachment 309278 [details] [diff] [review]
Patch to add in/inout param attribute annotations

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

Comment 3

10 years ago
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

10 years ago
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?
(Assignee)

Comment 5

10 years ago
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

10 years ago
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.
(Assignee)

Comment 7

10 years ago
(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.)
(Assignee)

Comment 8

10 years ago
Created attachment 317454 [details] [diff] [review]
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);
Attachment #309278 - Attachment is obsolete: true
Attachment #317454 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #317454 - Flags: review? → review?(benjamin)

Comment 9

10 years ago
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.
Attachment #317454 - Flags: review?(benjamin) → review+
(Assignee)

Comment 10

10 years ago
Pushed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: xpidl → XPCOM
QA Contact: xpidl → xpcom
You need to log in before you can comment on or make changes to this bug.