On 12/22/2016 08:14 PM, Bobby Holley wrote:
We've had this debate several times already, culminating in the attempt to
ban NS_ENSURE_* macros. It didn't work.
This is a bit different. One of the most common NS_ENSURE_ macros is SUCCESS
variant which explicitly has the
return value.
MOZ_TRY doesn't really hint at all, not even in its name, that it is doing
early return.
But I've always like even NS_ENSURE_STATE, so I guess I could get used to
code using Result, even though it at first sight looks rather verbose.
Subjectively, I think the reasons for the failure were:
(a) We want logging/warnings on exceptional return paths, especially the
ones we don't expect.
(b) We want to check every potential failure and propagate error codes
correctly.
(c) Trying to do (a) and (b) consistently without macros bloats every
function call to 5 lines of boilerplate.
If we do want to rehash the early-return macro discussion, we should do
that separately. As it stands, MOZ_TRY is necessary to give mozilla::Result
feature parity with nsresult. We don't want people to stick with nsresult
just because they like NS_ENSURE_SUCCESS.
bholley
On Wed, Dec 21, 2016 at 11:22 PM, Eric Rahm <er...@mozilla.com> 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 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
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
