1383174 - js::GetNonSyntacticGlobalThis() has an unused return at the end

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Andrew McCreight [:mccr8]]

This method iterates over the environment chain, looking for whatever. However, if it fails to find anything, it does not set the return value, but ends up returning true. That seems bad.

Comment 1

[Andrew McCreight [:mccr8]]

Jan, it looks like you wrote this code. Am I right, or am I missing something? Thanks.

Comment 2

[Jan de Mooij [:jandem]]

Note the while (true) {}, the loop is guaranteed to find something - we should never reach the |return true;| after it.

Not sure why this returns bool instead of void. Probably because it's called directly from JIT code and the machinery for that doesn't support void yet.

Comment 3

[Andrew McCreight [:mccr8]]

Ah, great. I figured that might be the case. I'll add a comment or crash or something in that case.

Comment 4

[Andrew McCreight [:mccr8]]

Oh, right, there's no break in the while(true) loop!

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1383174 - Make GetNonSyntacticGlobalThis return void.

This method always returns true.

Also, get rid of the unused return statement at the end of the
method. The loop is a while(true) and it has no break statements.

Review commit: https://reviewboard.mozilla.org/r/160622/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160622/

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8889594 [details]
Bug 1383174 - Make GetNonSyntacticGlobalThis return void.

https://reviewboard.mozilla.org/r/160622/#review166144

Nice

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3a843f6334d
Make GetNonSyntacticGlobalThis return void. r=jandem

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3a843f6334d