The key point for me is that we're hiding the return. I'm fine with the
more verbose
explicitly-return-and-make-it-easier-for-the-reviewer-to-call-out-issues
form. I understand this is going to have differing opinions -- I think
there's merit to more concise code -- but for the things I look at I much
prefer an explicit return. It makes both the writer and reviewer think
about the consequences. If MOZ_TRY just release asserted on error I
wouldn't have an issue.
On Wed, Dec 21, 2016 at 10:59 PM, Kris Maglione <kmagli...@mozilla.com>
wrote:
> On Wed, Dec 21, 2016 at 10:53:45PM -0800, Eric Rahm wrote:
>
>> I like the idea of pulling in some Rusty concepts, but I'm concerned about
>> adding yet another early return macro -- absolutely no arguments on the
>> new
>> type, just the |MOZ_TRY| macro. In practice these have lead to security
>> issues (UAF, etc) and memory leaks in the C++ world (I'm looking at you
>> NS_ENSURE). These aren't hypothetical issues, I've run into them both
>> working on memshrink and sec issues in Firefox.
>>
>
> We run into the same issues without those macros. The only real difference
> is that when you have to follow every call with:
>
> if (NS_WARN_IF(NS_FAILED(rv))) {
> return rv;
> }
>
> It's much easier to lose track of the allocation you did four calls ago
> that's now 20 lines away.
>
>
> On Wed, Dec 21, 2016 at 9:53 AM, Ted Mielczarek <t...@mielczarek.org>
>> wrote:
>>
>> On Wed, Dec 21, 2016, at 12:30 PM, Jason Orendorff wrote:
>>> > The implicit conversion solves a real problem. Imagine these two
>>> > operations
>>> > have two different error types:
>>> >
>>> > MOZ_TRY(JS_DoFirstThing()); // JS::Error&
>>> > MOZ_TRY(mozilla::pkix::DoSecondThing()); // pkix::Error
>>> >
>>> > We don't want our error-handling scheme getting in the way of using
>>> them
>>> > together. So we need a way of unifying the two error types: a shared
>>> base
>>> > class, perhaps, or a variant.
>>> >
>>> > Experience with Rust says that MOZ_TRY definitely needs to address this
>>> > problem somehow. C++ implicit conversion is just one way to go; we can
>>> > discuss alternatives in the bug.
>>>
>>> The `try` macro in Rust will auto-convert error types that implement
>>> `Into<E>`, AIUI, but that's not automatic for all error types. I haven't
>>> tried it, but I have seen multiple recommendations for the `error_chain`
>>> crate to make this smoother:
>>> https://docs.rs/error-chain/0.7.1/error_chain/
>>>
>>> It's basically just boilerplate to implement conversions from other
>>> error types. I wouldn't be surprised if something like that percolates
>>> into the Rust standard library at some point.
>>>
>>> -Ted
>>> _______________________________________________
>>> 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
>>
>
> --
> Kris Maglione
> Firefox Add-ons Engineer
> Mozilla Corporation
>
> It's always good to take an orthogonal view of something. It develops
> ideas.
> --Ken Thompson
>
>
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
