On Wed, Dec 21, 2016 at 11:22:24PM -0800, Eric Rahm wrote:
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've never been convinced by that argument. Reviewers accept
code with early return macros should be familiar enough with
them that they don't make it any harder to spot issues caused by
early return. On the flip side, explicit early return is a
pervasive enough pattern that I think most of us tend to just
filter it out as noise. But it's noise that makes the structure
of the code much harder to follow, which makes other errors much
easier to miss.
And, by the same token, this is part of MFBT, and comes along
with a lot of other helpers, and a general coding style, that
makes it much easier to RAII all the things, and to generally do
the right thing. In isolation, I might worry more about it
leading to new early return errors, but in context, I think it's
clearly a step in the right direction.
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
--
Kris Maglione
Firefox Add-ons Engineer
Mozilla Corporation
Life is too important to be taken seriously.
--Oscar Wilde
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
