[runtime] Refactor Object::Equals to make exception cases explicit.

[runtime] Refactor Object::Equals to make exception cases explicit.

R=littledan@chromium.org
BUG=v8:3593
LOG=n

Committed: https://crrev.com/e933b091df30a086e18f566c53381212fc38743c
Cr-Commit-Position: refs/heads/master@{#31642}

[Benedikt Meurer, 2015-10-28 04:49:15]

Hey Dan,

Object::Equals was intentionally written this way, with the invariant that you
stay in the loop unless you explicitly return. Using continue breaks that
invariant and makes it harder to read (IMHO). If you don't like the loop style
then it would be better to completely avoid it and use recursion (depth is
limited by the spec).
Please take a look.

Thanks,
Benedikt

[adamk, 2015-10-28 23:50:52]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2015-10-28 23:50:53]

drive-by comment (I was the reviewer on the patch that changed this): I found
the use of explicit "continue" clearer than the previous fall-through, which
required tracing many braces and else/ifs to figure out that not returning in a
given clause was equivalent to "continue". 

So there's definitely a stylistic argument on both sides (I'm also not sure how
"continue" breaks any invariant; the UNREACHABLE added at the end requires that
either continue or return is used).

If we decide to go back to the way it was before, a comment right above the
"while(true)" could at least explain the intended structure.

[Dan Ehrenberg, 2015-10-28 23:57:33]

On 2015/10/28 at 23:50:53, adamk wrote:
> drive-by comment (I was the reviewer on the patch that changed this): I found
the use of explicit "continue" clearer than the previous fall-through, which
required tracing many braces and else/ifs to figure out that not returning in a
given clause was equivalent to "continue". 
> 
> So there's definitely a stylistic argument on both sides (I'm also not sure
how "continue" breaks any invariant; the UNREACHABLE added at the end requires
that either continue or return is used).
> 
> If we decide to go back to the way it was before, a comment right above the
"while(true)" could at least explain the intended structure.

Right, the reason I changed this was to make it more clear, exactly as Adam is
saying. continue indicates that all we are doing is running == again, as in the
spec, but without continue, more code could run in the loop (though the contents
are written carefully to ensure that does not happen). How is this form that you
are changing it back to more explicit? You do stay in the loop this way unless
you explicitly return.

[Benedikt Meurer, 2015-10-29 03:57:06]

IMHO if you need continue several times in a loop then that's a good indicator
that there's something wrong with the loop. Esp if you introduce code patterns
like

if (operation) continue;
else return;

in a loop whose invariant is that you stay inside the loop unless you return.
Seeing continue in such a loop indicates that you are skipping something, but
the only code you skip is UNREACHABLE().

So if you think that it's hard to read the loop, then I'm fine with changing
that to recursion (which would match the spec exactly). But I'd strongly
discourage the use of continue here.

[Dan Ehrenberg, 2015-10-29 04:17:22]

lgtm

OK, I don't want to go back and forth about this stylistic issue too much.
Recursion carries potential technical cost, and this patch doesn't make the code
unreadable, so it's fine with me.

[Benedikt Meurer, 2015-10-29 04:39:34]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2015-10-29 04:39:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407233013/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407233013/1

[commit-bot: I haz the power, 2015-10-29 04:41:02]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-10-29 04:41:30]

Patchset 1 (id:??) landed as
https://crrev.com/e933b091df30a086e18f566c53381212fc38743c
Cr-Commit-Position: refs/heads/master@{#31642}