459452 - Add support for optional arg count for IDL methods.

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Johnny Stenback (:jst)]

Attachment: Add support for [optional_argc]

It'd be neat, and it would let us remove a bunch of JS specific code, if we had a way to tell in C++ code how many of the optional arguments for a given method were actually passed in by the caller. I have a patch that adds support for a per-method [optional_argc] flag that makes us generate an additional argument to the C++ implementations of the methods. That argument is of type PRUint8, and contains the number of optional arguments that were given by the caller. The argument will be after the last optional argument in the signature, but before the return value, which complicates XPConnect's argument handling a bit, but not too bad. Please see the attached patch.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Sampling of what the above aptch would lets us do.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 342669 [details] [diff] [review]
Add support for [optional_argc]

>diff -r 8c75834cc400 js/src/xpconnect/src/xpcwrappednative.cpp
>--- a/js/src/xpconnect/src/xpcwrappednative.cpp	Mon Oct 06 14:27:24 2008 -0700
>+++ b/js/src/xpconnect/src/xpcwrappednative.cpp	Fri Oct 10 16:59:42 2008 -0700
>@@ -1942,6 +1942,8 @@ XPCWrappedNative::CallMethod(XPCCallCont
>     const nsXPTMethodInfo* methodInfo;
>     uint8 requiredArgs;
>     uint8 paramCount;
>+    uint8 wantsOptArgc;
>+    uint8 optArgcIndex = PR_UINT8_MAX;
>     jsval src;
>     nsresult invokeResult;
>     nsID param_iid;
>@@ -2054,16 +2056,22 @@ XPCWrappedNative::CallMethod(XPCCallCont
>         goto done;
>     }
> 
>+    wantsOptArgc = methodInfo->WantsOptArgc() ? 1 : 0;
>+
>     // XXX ASSUMES that retval is last arg. The xpidl compiler ensures this.
>     paramCount = methodInfo->GetParamCount();
>     requiredArgs = paramCount;
>     if(paramCount && methodInfo->GetParam(paramCount-1).IsRetval())
>         requiredArgs--;
>-    if(argc < requiredArgs)
>+
>+    if(argc < requiredArgs || wantsOptArgc)
>     {
>+        if(wantsOptArgc)
>+            optArgcIndex = requiredArgs;

Not really sure why the outer |if| exists here. Is it because we want to save cycles and not call ->GetParam(..).IsOptional()?

>@@ -2397,11 +2405,29 @@ XPCWrappedNative::CallMethod(XPCCallCont
>         }
>     }
> 
>+    // Fill in the optional_argc argument
>+    if(wantsOptArgc)
>+    {
>+        nsXPTCVariant* dp = &dispatchParams[optArgcIndex];
>+
>+        if(optArgcIndex != paramCount)
>+        {
>+            // The method has a return value, the return value must be
>+            // last so push it out one so that we'll have room to
>+            // insert the optional argc argument.
>+            dispatchParams[paramCount] = *dp;
>+        }

Don't you need to also push the varargs parameter too if it exists? I.e. can paramCount be bigger than optArgcIndex+1?

I don't see how varargs are handled here at all...

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

r=me with that addressed or fixed.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Would be great to get this fixed.

Comment 5

[Boris Zbarsky [:bzbarsky]]

It'd be nice to drive this in, esp. with quickstub support.

Comment 6

[Peter Van der Beken [:peterv]]

Yeah, working on this. Have a patch that adds quickstubs support too.

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to comment #2)
> Not really sure why the outer |if| exists here. Is it because we want to save
> cycles and not call ->GetParam(..).IsOptional()?

I think so, yes.

> Don't you need to also push the varargs parameter too if it exists? I.e. can
> paramCount be bigger than optArgcIndex+1?

>     requiredArgs = paramCount;
>     if(paramCount && methodInfo->GetParam(paramCount-1).IsRetval())
>         requiredArgs--;
> 
>     if(argc < requiredArgs || wantsOptArgc)
>     {
>         if(wantsOptArgc)
>             optArgcIndex = requiredArgs;

Looks like optArgcIndex is either == paramCount or == paramCount - 1 (so optArgcIndex <= paramCount).

> I don't see how varargs are handled here at all...

XPConnect doesn't support varargs, so not sure what you're asking.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Add quickstubs support v1

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Use optional_argc v1

Updated to trunk. I'm reviewing this.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Use optional_argc v1

Changed some interface uuids, undid some unrelated changes, cleaned up some whitespace.

Comment 11

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/cfa396fc6a5d
http://hg.mozilla.org/mozilla-central/rev/aeb6801640e6
http://hg.mozilla.org/mozilla-central/rev/bd4b2f8428b3

Comment 12

[lkcl]

if this has been committed i strongly advise you to back it out, now.
you will run into significant complications down the line.
that's all i'm going to say, because, being blunt: like all free
software developers i've ever advised, i'm not anticipating that
you actually listen to my advice.  the fact the correct technical
solution has already been dismissed, by mozilla developers, lends
weight to this anticipated outcome.  however, my duty towards
free software compels me to make this advice anyway: back out these
changes and use the correct technical solution: coclasses (see
design of COM (DCOM) for how to implement them)  you _almost_ have a
complete and correct COM-like implementation, and it is the lack
of coclasses that is forcing these kinds of half-baked "solutions"
such as this one.

but - you know better, and you've dismissed coclasses already, so...
good luck with screwing up the infrastructure behind the otherwise
excellent mozilla codebase.  call me when you want some help sorting
out the mess.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Having just read through what coclasses are, I don't see how they address this use case.

Comment 14

[lkcl]

then keep investigating, boris.

they're effectively a way to do operator/function overloading, just like
in c++, but with the distinct advantage that you can do the overloading
in absolutely any language, and call them from absolutely any language.

if you happen to write the COM component in a high level language which
_does_ support operator/function overloading, you can very nicely add
in "transparent" mapping onto the coclass underlying functions, such
that you don't even know that they are there.

if you have a _dynamic_ language on top of the COM component, you can
even do dynamic runtime binding, with introspection, that takes away
literally all of the work.  all you do is "point" the dynamic-binding
library at the COM interface and it "gets on with the job", even though
there's coclasses.

basically what you do is you subdivide the IDL files into separate
IDL files, each of which contains the functions with the "clashing"
names.

the coclass is then the c++ equivalent of "multiple inheritance",
re-merging all those "clashing" names back into one "class".

at runtime you then have two choices - exactly as you do when you
have c++ "multiple inheritance":

* let the runtime decide what to do, given the number and type
  of function arguments (this is expensive, but less work for
  the programmer).

* explicitly access the interface a la "InterfaceName::functionname"
  by using COM / XPCOM QueryInterface.  this can be hidden behind
  macros etc. etc.

unfortunately, someone from the mozilla foundation has already
declared this tried, tested and proven method, which microsoft
implemented over 20 years ago and used to great success to build
their entire empire on, to be "****".

however, the method recommended here, which you should have already
backed out, is an absolute dog's dinner, and can be charitably
classed as amateurish, at best.

you will encounter _enormous_ difficulties with it.

Comment 15

[Boris Zbarsky [:bzbarsky]]

lkcl, the method you describe has one unfortunate drawback: fairly high performance cost of the part you describe as happening at runtime.  Since the primary use case for the functionality in question is calls from web JS into C++, and since performance is a key design criterion here, coclasses do not in fact seem to fit the bill.

Fundamentally, you seem to be confused about the goals of this patch.  It's not there to provide generic method overloading capabilities (though those would be nice too, depending on where webidl goes).  It's there to solve a specific problem.

Comment 16

[lkcl]

(In reply to comment #15)
> lkcl, the method you describe has one unfortunate drawback: fairly high
> performance cost of the part you describe as happening at runtime.

 wrong.  there are two well-known approaches to dealing with coclasses:

 1) explicit binding
 2) lazy (dynamic) binding

 1) is fast.
 2) is obviously slow.

>  Since the
> primary use case for the functionality in question is calls from web JS into
> C++, and since performance is a key design criterion here, coclasses do not in
> fact seem to fit the bill.

 where are the design documents and comparative analyses showing this to be the case?

> Fundamentally, you seem to be confused about the goals of this patch.  It's not
> there to provide generic method overloading capabilities (though those would be
> nice too, depending on where webidl goes). 

 down the toilet, as far as i can make out, with bitty little "fixes".

 you could go _so_ far if you actually completed this work and followed the well-known, proven, tested path that microsoft has shown, demonstrated and relied on to make a runaway success of its OS, for the last 20 years.

 well, i don't care now.  go ahead.  do whatever you like.  you know far
 better than i do, obviously, so there's absolutely no point in me opening my
 mouth - i'm completely wasting my time.

> It's there to solve a specific
> problem.

 and in the meantime, it screws up python-xpcom.

 please revert this patch or place it into a branch, pending completion of
 work on getting python-xpcom to work with it.

Comment 17

[Dave Townsend [:mossop]]

Attachment: IID rev for nsIXMLHttpRequest

The IID for nsIXMLHttpRequest should have been changed with the change to the open method.

Comment 18

[Dave Townsend [:mossop]]

Landed the IID rev: http://hg.mozilla.org/mozilla-central/rev/6235ed75968a

Comment 19

[lkcl]

btw i just wanted to let people know: i spoke to todd from activestate and he explained that he is taking care of python-xpcom.  i'm extremely grateful for this, because it means that developers writing applications to run under pyjamas-desktop (and also hulahop to a lesser extent in OLPC) can "keep going" without having to "roll their own" version of xulrunner.  pyjamas-desktop developers are python developers, not c++ experts.  todd's work basically saves the pyjamas-desktop project in the free software world, so really big thank you.

l.

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://developer.mozilla.org/en/XPIDL

Comment 22

[lkcl]

https://developer.mozilla.org/en/XPIDL

optional_argc 	Y 	N 	Adds an additional PRUint8 _argc parameter to the C++ implementation

i cannot express how much of a deeply unimpressively bad idea this decision is.  unfortunately you're going to find out, when it adversely impacts the underlying c++ implementation.  consider yourselves warned, and when you're ready to listen, i will be happy to advise.