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 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...

r=me with that addressed or fixed.

Would be great to get this fixed.

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

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

(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.

Updated to trunk. I'm reviewing this.

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

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

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.

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

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.

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.

(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.

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

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

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.

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.