Closed
Bug 780970
Opened 12 years ago
Closed 12 years ago
Add [infallible] attribute to XPIDL
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
Details
Attachments
(1 file, 2 obsolete files)
5.84 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
I don't think it would be particularly hard to add an [infallible] attribute to XPIDL. This would translate the following IDL [infallible] readonly attribute long foo; to either the following C++ // Option A NS_IMETHOD PRInt32 GetFoo() = 0; PRInt32 GetFoo() { PRInt32 result = 0; MOZ_ASSERT(NS_SUCCEEDED(GetFoo(&result))); return result; } or the following C++ // Option B virtual PRInt32 GetFoo() = 0; nsresult GetFoo(PRInt32* aFoo) { NS_ENSURE_ARG_POINTER(aFoo); *aFoo = GetFoo(); return NS_OK; } This would make interacting with XPCOM interfaces from C++ /so much better/. For starters, I'd like to implement this only for attributes which return primitives. We could of course expand it to attributes returning objects, and to interface methods. Option B is cleaner, so although it's marginally trickier than option A, I'm trying it now...
Assignee | ||
Comment 1•12 years ago
|
||
This seems to generate reasonable header files. I haven't tested it. I'm concerned because option B (which this patch implements) results in two virtual calls for every attribute access, where we used to have only one. But glandium may make a saving throw.
Comment 2•12 years ago
|
||
What's the goal here? Faster scriptable methods or just better C++ accessibility? Would this attribute imply that the interface is [builtinclass] (since any interface implemented by script may fail in the impl)? If you change the virtual signature at all currently, the methods will not be scriptable (or will only be scriptable via quickstubs/new DOM bindings, not via long-form xpconnect). If you don't care about scriptability, why not just use [notxpcom]? Changing xptcall to work with methods of the form NS_IMETHOD_(void) Method(PRInt32* result); would be relatively straightforward or perhaps wouldn't require any xptcall changes. Changing the actual return type would be a lot more complicated, especially for any non-primitive types that require allocation/resource management.
Assignee | ||
Comment 3•12 years ago
|
||
> What's the goal here? Faster scriptable methods or just better C++ accessibility? I'm only concerned with C++ ergonomics. It's my understanding that we're moving away from XPCOM method dispatch wherever we care about performance. > Would this > attribute imply that the interface is [builtinclass] (since any interface implemented by script > may fail in the impl)? Option B would imply this, I guess, yes. > Changing the actual return type would be a lot more complicated, especially for any > non-primitive types that require allocation/resource management. I was thinking that we'd support [infallible] only on attributes which return primitives, to start with. I talked with jst about this for a bit, and concluded that option A is probably the way to go. Option B changes the class's vtable, so we'd have to make XPCOM aware of the changes. Plus, option B doesn't work with interfaces implemented in JS, while option A works fine.
Assignee | ||
Comment 4•12 years ago
|
||
This implements option A. I'll attach an example of generated output in a moment.
Attachment #649946 -
Flags: review?(khuey)
Assignee | ||
Comment 5•12 years ago
|
||
/* [infallible] readonly attribute boolean isBrowserElement; */ NS_IMETHOD GetIsBrowserElement(bool *aIsBrowserElement) = 0; bool GetIsBrowserElement() { bool result; nsresult rv = GetIsBrowserElement(&result); MOZ_ASSERT(NS_SUCCEEDED(rv)); return result; }
Comment 6•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #3) > I talked with jst about this for a bit, and concluded that option A is > probably the way to go. Option B changes the class's vtable, so we'd have > to make XPCOM aware of the changes. Plus, option B doesn't work with > interfaces implemented in JS, while option A works fine. More than that, it would add entries to the vtable. Option A has the advantage of being inlinable and, in the end, would yield more or less the same code (module the MOZ_ASSERT) as what it would replace. I couldn't think of anything better for what you were after. I was confused with the way you wrote option A initially, though.
Comment 7•12 years ago
|
||
Comment on attachment 649946 [details] [diff] [review] Patch, v1 Yeah, I was confused about option A also. I think this is ok, but I do want [infallible] to require that the interface be marked [builtinclass] in xpidl.py
Assignee | ||
Comment 8•12 years ago
|
||
> but I do want [infallible] to require that the interface be marked [builtinclass] in xpidl.py
Under what circumstances might JS code that doesn't (transitively across invoked code) throw an exception cause us to return an nserror code?
Comment 9•12 years ago
|
||
Well, you can sometimes make xpconnect throw an exception (perhaps by holding a reference to a dead compartment), but throwing a JS exception is how you normally end up with failure nsresults from scripted interfaces...
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → justin.lebar+bug
Assignee | ||
Comment 10•12 years ago
|
||
Don't allow [infallible] on non-[builtinclass] interfaces.
Attachment #650583 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•12 years ago
|
Attachment #649777 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #649946 -
Attachment is obsolete: true
Attachment #649946 -
Flags: review?(khuey)
Assignee | ||
Updated•12 years ago
|
Attachment #650583 -
Flags: review?(ted.mielczarek) → review?(khuey)
Comment 11•12 years ago
|
||
Comment on attachment 650583 [details] [diff] [review] Patch, v2 Review of attachment 650583 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/idl-parser/header.py @@ +296,5 @@ > + { > + %(realtype)sresult; > + nsresult rv = %(nativename)s(%(argnames)s&result); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); > + return result; This will warn. You could use MOZ_ALWAYS_TRUE or DebugOnly
Comment on attachment 650583 [details] [diff] [review] Patch, v2 Review of attachment 650583 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/idl-parser/header.py @@ +291,5 @@ > > """ > > +attr_infallible_tmpl = """\ > + %(realtype)s%(nativename)s(%(args)s) Please make this 'inline'.
Attachment #650583 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 13•12 years ago
|
||
I can land this once we get bug 784436, which actually /uses/ [infallible], taken care of.
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6c7efe053241
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•