382034 - support optional arguments in idl

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Neil Deakin]

Attachment: support optional flag

For bug 380813, it would be nice if idl could support optional arguments.

For example:
  AString readString(in unsigned long aCount, [optional] in AString aCharSet);

which would pass something nullish if the argument wasn't supplied.

Not sure if this will be a performance hit, but this patch supports the optional flag for JS callers, but doesn't affect native callers, which will still just call as before.

Comment 1

[[e-mail]]

(In reply to comment #0)
> For example:
>   AString readString(in unsigned long aCount, [optional] in AString aCharSet);
> 
> which would pass something nullish if the argument wasn't supplied.
> 
> Not sure if this will be a performance hit, but this patch supports the
> optional flag for JS callers, but doesn't affect native callers, which will
> still just call as before.
C++ callers are easy ;-) Just add
%{ C++
  nsresult ReadString(PRUint32 aCount) {
    return ReadString(aCount, EmptyString());
  }
}%

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

shaver: didn't you say you thought you had r+'d this?

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 266090 [details] [diff] [review]
support optional flag

I swear I've reviewed this patch like 3 times now.  It looks good each time!

Only comment I can remember is about making sure that we don't need to rev any of the XPT version numbers, but IIRC this sort of flag addition is safe.  r+sr=shaver, many apologies for the delay.

Comment 4

[Neil Deakin]

Attachment: Fix a crash

This is an updated version of this patch which fixes a crash in two of the automated tests. The crash is caused when calling an implied toString on an object.

Comment 5

[Mike Shaver (:shaver emeritus)]

Comment on attachment 269506 [details] [diff] [review]
Fix a crash

Aye. r+sr=shaver

Comment 6

[jag (Peter Annema)]

Comment on attachment 269506 [details] [diff] [review]
Fix a crash

I'm probably reading this wrong, but in

+          jsval arg = paramInfo.IsOptional() && argc < i ? JSVAL_NULL : argv[i];

shouldn't |argc < i| be |argc <= i|? Though I think it's easier to understand as

+          jsval arg = i >= argc && paramInfo.IsOptional() ? JSVAL_NULL : argv[i];

Though that looks wrong too. If |i >= argc| then currently the param is either IsOptional() or IsRetVal(). In all cases you rule out it's not a retval (you're either in the else case of an IsOut() or you explicitly checked !IsRetVal()), so it looks like you can drop the IsOptional() test. If there's ever another type of param that would decrease |requiredArgs| then that assumption doesn't hold anymore, but with the |&& IsOptional()| test you'd end up using argv[i], which doesn't seem correct either, so perhaps an assertion that it is indeed optional wouldn't be a bad thing.

Nit: all the code surrounding your changes doesn't put a space between if/while and the opening parenthesis, please follow that style, unless that rule doesn't apply (anymore?) in JS land.

Comment 7

[Neil Deakin]

Checked in. I'll look into some additional cleanup to address comment 6.

Comment 8

[jag (Peter Annema)]

I would say the |argc < i| is a logic bug. Hopefully I'm just misunderstanding the code, but it looks like you'll end up using argv[i] instead of JSVAL_NULL if e.g. you have a function with one required arg and one optional arg, and it's called with just one parameter. Then argc == 1, and for i == 1 you'll get IsOptional() is true and argc < i is false, resulting in argv[i], when you want JSVAL_NULL.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So... I think jag is right and that should be a <= at the very least.

Of course we'd know for sure if we had tests...

And yes, the patch doesn't follow the XPConnect code style, which should be fixed.

Comment 10

[Neil Deakin]

OK, I looked at this again, and I think it should be <= as well. I'll create a patch for this.

Comment 11

[jag (Peter Annema)]

Neil, what do you think about dropping the (redundant) test for IsOptional() and asserting that instead?

Comment 12

[Neil Deakin]

Attachment: like so?

change to check >= and assertion with IsOptional check

Comment 13

[jag (Peter Annema)]

Comment on attachment 270779 [details] [diff] [review]
like so?

>Index: js/src/xpconnect/src/xpcwrappednative.cpp
>===================================================================
>+          NS_ASSERTION(i < argc || paramInfo.IsOptional(),
>+                       "Expected either enough arguments or an optional argument");
>+          jsval arg = i >= argc ? JSVAL_NULL : argv[i];

Looks good, though |jsval arg = i < argc ? argv[i] : JSVAL_NULL;| is the more idiomatic form. I should've suggested that to begin with, sorry.

>         if(paramInfo.IsRetval())
>         {
>             if(!ccx.GetReturnValueWasSet())
>                 ccx.SetRetVal(v);
>         }
>-        else if (!paramInfo.IsOptional() || argc > i)
>+        else
>         {
>+            NS_ASSERTION(i < argc || paramInfo.IsOptional(),
>+                         "Expected either enough arguments or an optional argument");
>+
>             // we actually assured this before doing the invoke
>             NS_ASSERTION(JSVAL_IS_OBJECT(argv[i]), "out var is not object");

Why is it safe to remove |if (i < argc)| here?

And while you're in here, can you address the "space between if/for and '('" nits from your previous patch, to match the file's style?

I'm not a peer for xpconnect, so asking shaver for sr.

Comment 14

[jag (Peter Annema)]

Attachment: Added back (i < argc) check, added else case with assertion, addressed my nits

Like the previous patch, but added back the check for |i < argc| for setting the |value| property on out parameters, otherwise we might read off the end of argv.

We also don't want to treat the case were |i >= argc && paramInfo.IsOptional()| as if the caller hadn't provided an object or no argument at all (exception gets thrown?), that would conflict with the parameter being optional. So, this, plus an assertion in the else case seems the way to go.

Comment 15

[Eric Shepherd [:sheppy]]

Documenting this should be part of an effort to migrate the XPIDL docs to MDC.  I don't know when that will happen, but most likely not before Fx3 ships, unless someone else wants to do it.

Comment 16

[Nickolay_Ponomarev]

For the reference, the to-be-migrated docs are linked from http://developer.mozilla.org/en/docs/XPIDL

BTW, we do have one doc on xpidl syntax on MDC:
http://developer.mozilla.org/en/docs/XPIDL:Syntax

Comment 17

[Eric Shepherd [:sheppy]]

Removing doc needed tag in favor of an actual bug that encompasses this work (bug 399537)

Comment 18

[jag (Peter Annema)]

Checking in xpcwrappednative.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,v  <--  xpcwrappednative.cpp
new revision: 1.164; previous revision: 1.163
done