Re: New XPIDL attribute: [infallible]
On Fri, 24 Aug 2012 12:20:58 -0700, Justin Lebar wrote:
inline int32_t GetFoo() {
int32_t result = 0;
nsresult rv = GetFoo(result);
MOZ_ASSERT(NS_SUCCEEDED(rv));
return result;
}
so that we never return uninitialized memory in release builds. 0 may
or may not be right, but it should be no worse than a random value!
Initialization to a meaningless value is worse because it means
that valgrind (and perhaps other analysis tools) won't detect when
GetFoo is misbehaving.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: New XPIDL attribute: [infallible]
Justin Lebar wrote: So now you can do nsCOMPtrnsIFoo foo; int32_t f = foo-GetFoo(); Why was I expecting this to be Foo()? (Perhaps unreasonably.) I rejected the first approach because it meant that every call to GetFoo from XPCOM would need to go through two virtual calls: GetFoo(int32_t*) and then GetFoo(). And also because MSVC would have messed up the vtable. -- Warning: May contain traces of nuts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: New XPIDL attribute: [infallible]
Justin Lebar wrote:
%{C++
inline int32_t GetFoo() {
int32_t result;
nsresult rv = GetFoo(result);
MOZ_ASSERT(NS_SUCCEEDED(rv));
return result;
}
%}
Alternative approach?
inline int32_t Foo(int32_t result = 0) {
GetFoo(result);
return result;
}
--
Warning: May contain traces of nuts.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: New XPIDL attribute: [infallible]
On 08/24/2012 02:42 AM, Neil wrote: Justin Lebar wrote: So now you can do nsCOMPtrnsIFoo foo; int32_t f = foo-GetFoo(); Why was I expecting this to be Foo()? (Perhaps unreasonably.) Yeah, it should be Foo(). File a bug? I rejected the first approach because it meant that every call to GetFoo from XPCOM would need to go through two virtual calls: GetFoo(int32_t*) and then GetFoo(). And also because MSVC would have messed up the vtable. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: New XPIDL attribute: [infallible]
nsCOMPtrnsIFoo foo; int32_t f = foo-GetFoo(); Why was I expecting this to be Foo()? (Perhaps unreasonably.) Yeah, it should be Foo(). File a bug? I considered Foo(), but my concern was that, when we extend this to attributes which return interfaces (e.g. nsIFoo), then Foo() versus GetFoo() has a particular meaning (in parts of our code): Foo() must never return null. But I was expecting that an infallible attribute could correctly return null. We can tweak this: attributes which return primitives are always Foo(), and attributes which return interfaces are GetFoo() unless we also have [nevernull] on the attribute. I'm not sure if the complexity there is worth the cost. I rejected the first approach because it meant that every call to GetFoo from XPCOM would need to go through two virtual calls: GetFoo(int32_t*) and then GetFoo(). And also because MSVC would have messed up the vtable. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
New XPIDL attribute: [infallible]
I landed bug 780970 today, which adds [infallible], a new XPIDL attribute.
You can only use [infallible] within interfaces marked as
[builtinclass] (which means the interface may not be implemented in
JS), and [infallible] is only applicable at the moment on XPIDL
attributes which return primitives. For example
[builtinclass]
interface nsIFoo {
[infallible] attribute long foo; // ok
[infallible] attribute nsIBar bar; // ERROR, nsIBar is not a primitive.
[infallible] long doCalculation(); // ERROR, doCalculation is a
method, not an attribute.
}
[infallible] is a promise that your attribute's getter will never
return anything other than NS_OK, and, as a corollary, that the getter
will always assign a value to its outparam.
When you mark an attribute as infallible, we generate a helper in the
header file which lets you call the attribute's getter without using
an outparam. For instance, if we ignore the erroneous lines in the
interface above, it is equivalent to:
[builtinclass]
interface nsIFoo {
attribute long foo;
%{C++
inline int32_t GetFoo() {
int32_t result;
nsresult rv = GetFoo(result);
MOZ_ASSERT(NS_SUCCEEDED(rv));
return result;
}
%}
}
So now you can do
nsCOMPtrnsIFoo foo;
int32_t f = foo-GetFoo();
A few things to note:
1. The MOZ_ASSERT is debug-only. That means that in release builds,
if GetFoo(int32_t*) returns an error code and doesn't set *result,
GetFoo()'s return value will be undefined. [infallible] is a promise
that we can't (at the moment) statically enforce, but it's a promise
nonetheless! (If we ever get static analysis back up and running,
that infallible methods never fail would be a great thing to check.)
2. [infallible] does not change the implementation of nsFoo::GetFoo in
C++. You still write the same NS_IMETHODIMP nsFoo::GetFoo(int32_t*)
that you used to, and you can still call GetFoo(int32_t*) if you want
to. The only difference is that now you may also call GetFoo().
For an example of how to use [infallible], see [1, 2].
I encourage you to consider using [infallible] in new code for
attributes whose getters never fail. Not only is it cleaner to call
such methods, but using [infallible] also draws attention to those
attributes which /may/ fail.
If this experiment goes well, I think we should expand [infallible] to
methods and to methods/attributes which return interfaces -- that is,
the two ERROR lines in the interface at the top would no longer be
errors. There's no technical reason I can think of not to do this; I
just wanted to start small.
One reasonable question to ask at this point is: Why not make GetFoo()
the canonical implementation and either implement GetFoo(int32_t*) in
terms of GetFoo(), or otherwise modify XPCOM to call GetFoo()
directly?
There's some discussion of this in the bug, but basically: I rejected
the first approach because it meant that every call to GetFoo from
XPCOM would need to go through two virtual calls: GetFoo(int32_t*) and
then GetFoo(). Moreover, this approach doesn't let us avoid modifying
XPCOM, because we'd be adding a new virtual method to nsIFoo. This
modifies nsIFoo's vtable, so XPCOM would need to be aware of this.
I think the second approach, of eliminating the outparam version of
GetFoo altogether, is definitely cleaner than what I've implemented.
It could yield a perf improvement, and it also means that the compiler
will enforce the rule that GetFoo can't fail. But modifying XPCOM to
call an entirely new type of method was not an exciting proposition to
me. Anyway, we can always make this modification at a later date.
Happy hacking,
-Justin
[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0bf4b676da
[2] https://hg.mozilla.org/integration/mozilla-inbound/rev/eb81ebe55d99
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: New XPIDL attribute: [infallible]
On 2012年08月23日 18:11, Justin Lebar wrote: I encourage you to consider using [infallible] in new code for attributes whose getters never fail. Not only is it cleaner to call such methods, but using [infallible] also draws attention to those attributes which /may/ fail. Before each time using [infallible] for new attribute, discussion whether the interface should be builtinclass or not will be opened? Or you also encourage to make the interface builtinclass anyway? ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
