1325986 - Use ScopeExit in a few more places

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Going through some old patches, I found this one where I was beefing up MakeScopeExit with another feature (enable/disable) and using it in a couple of places. But I noticed that some of the changes didn't even require the new feature. And the ones that did weren't very compelling. So I ditched the new feature, and will try to land this small patch with a couple of new uses.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Use ScopeExit in a few more places

It uses some weird new syntactical goop, but it seems harder to mess up.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8822095 [details] [diff] [review]
Use ScopeExit in a few more places

Review of attachment 8822095 [details] [diff] [review]:
-----------------------------------------------------------------

r+ to landing the WasmSignalHandlers.cpp changes.

::: js/src/builtin/AtomicsObject.cpp
@@ +996,5 @@
>          auto sliceEnd = finalEnd.map([&](mozilla::TimeStamp& finalEnd) {
>              auto sliceEnd = mozilla::TimeStamp::Now() + maxSlice;
>              if (finalEnd < sliceEnd)
>                  sliceEnd = finalEnd;
>              return sliceEnd;

IIUC, this return path would not have set the state_ to Idle before this patch but will set it to Idle with the patch.  I don't know what the intended behavior is here (and why are we returning a time value as a boolean....); probably best to split off a separate patch and have Lars review.

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Luke Wagner [:luke] from comment #2)
> ::: js/src/builtin/AtomicsObject.cpp
> @@ +996,5 @@
> >          auto sliceEnd = finalEnd.map([&](mozilla::TimeStamp& finalEnd) {
> >              auto sliceEnd = mozilla::TimeStamp::Now() + maxSlice;
> >              if (finalEnd < sliceEnd)
> >                  sliceEnd = finalEnd;
> >              return sliceEnd;
> 
> IIUC, this return path would not have set the state_ to Idle before this
> patch but will set it to Idle with the patch.  I don't know what the
> intended behavior is here (and why are we returning a time value as a
> boolean....); probably best to split off a separate patch and have Lars
> review.

Heh. This isn't doing at all what you're thinking it does. That's not a return from the function (which, as you say, would be nonsense since the function returns a bool). It is just a return in the lambda passed to finalEnd.map. So this code isn't going to change state_ at all.

But sure, I'll toss it over to Lars.

And... hm. This is the first I've seen Maybe::map used. It's, uh, weird. Simpler example:

    auto finalEnd = timeout.map([](mozilla::TimeDuration& timeout) {
        return mozilla::TimeStamp::Now() + timeout;
    });

It's a simple assignment (finalEnd = Now() + timeout) with Nothing-ness propagation.

Gonna have to keep those darn Rustaceans off *my* lawn.

Comment 4

[Luke Wagner [:luke]]

Ah hah, right, I missed the lambda.

Comment 5

[Lars T Hansen [:lth]]

Comment on attachment 8822095 [details] [diff] [review]
Use ScopeExit in a few more places

Review of attachment 8822095 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60fa55d96e2c
Use ScopeExit in a few more places, r=luke,lth

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=71702688&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/2781e8137927e3970b142807a47d614650611966

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85fec0c2e037
Use ScopeExit in a few more places, r=luke,lth

Comment 9

[Steve Fink [:sfink] [:s:]]

Yes, there was an osx-only build failure. Which is weird, because I kind of agree with the osx compiler here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fda28e136ab610d57882e45e65e9e5a191b8c18e seems to say I fixed that problem, though I probably rebased onto a bad rev for SM(arm).

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85fec0c2e037