Closed Bug 382034 Opened 15 years ago Closed 15 years ago

support optional arguments in idl

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha6

People

(Reporter: enndeakin, Assigned: enndeakin)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch support optional flag (obsolete) — Splinter Review
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.
Attachment #266090 - Flags: superreview?(shaver)
Attachment #266090 - Flags: review?(shaver)
(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());
  }
}%
Keywords: dev-doc-needed
OS: Mac OS X → All
Hardware: PC → All
shaver: didn't you say you thought you had r+'d this?
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.
Attachment #266090 - Flags: superreview?(shaver)
Attachment #266090 - Flags: superreview+
Attachment #266090 - Flags: review?(shaver)
Attachment #266090 - Flags: review+
Attached patch Fix a crashSplinter Review
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.
Attachment #266090 - Attachment is obsolete: true
Attachment #269506 - Flags: superreview?(shaver)
Attachment #269506 - Flags: review?(shaver)
Comment on attachment 269506 [details] [diff] [review]
Fix a crash

Aye. r+sr=shaver
Attachment #269506 - Flags: superreview?(shaver)
Attachment #269506 - Flags: superreview+
Attachment #269506 - Flags: review?(shaver)
Attachment #269506 - Flags: review+
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.
Checked in. I'll look into some additional cleanup to address comment 6.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9alpha6
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.
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.
OK, I looked at this again, and I think it should be <= as well. I'll create a patch for this.
Neil, what do you think about dropping the (redundant) test for IsOptional() and asserting that instead?
Attached patch like so? (obsolete) — Splinter Review
change to check >= and assertion with IsOptional check
Attachment #270779 - Flags: superreview?(jag)
Attachment #270779 - Flags: review?(jag)
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.
Attachment #270779 - Flags: superreview?(shaver)
Attachment #270779 - Flags: superreview?(jag)
Attachment #270779 - Flags: review?(jag)
Attachment #270779 - Flags: review+
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.
Attachment #270779 - Attachment is obsolete: true
Attachment #274325 - Flags: superreview?(shaver)
Attachment #274325 - Flags: review?(enndeakin)
Attachment #270779 - Flags: superreview?(shaver)
Attachment #274325 - Flags: review?(enndeakin) → review+
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.
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
Removing doc needed tag in favor of an actual bug that encompasses this work (bug 399537)
Keywords: dev-doc-needed
Blocks: 399537
Attachment #274325 - Flags: superreview?(shaver) → superreview?(jst)
Attachment #274325 - Flags: superreview?(jst) → superreview+
Attachment #274325 - Flags: approval1.9?
Attachment #274325 - Flags: approval1.9? → approval1.9+
Checking in xpcwrappednative.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,v  <--  xpcwrappednative.cpp
new revision: 1.164; previous revision: 1.163
done
You need to log in before you can comment on or make changes to this bug.