Bug 206157 - [JSC] Consistently use "var" in builtin JS
Summary: [JSC] Consistently use "var" in builtin JS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-12 15:06 PST by Yusuke Suzuki
Modified: 2020-01-13 11:45 PST (History)
8 users (show)

See Also:


Attachments
Patch (59.80 KB, patch)
2020-01-12 15:07 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (56.87 KB, patch)
2020-01-12 15:14 PST, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-01-12 15:06:10 PST
[JSC] Consistently use "var" in builtin JS
Comment 1 Yusuke Suzuki 2020-01-12 15:07:04 PST
Created attachment 387493 [details]
Patch
Comment 2 Yusuke Suzuki 2020-01-12 15:14:10 PST
Created attachment 387494 [details]
Patch
Comment 3 Mark Lam 2020-01-12 15:31:28 PST
Comment on attachment 387493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387493&action=review

> Source/JavaScriptCore/builtins/RegExpPrototype.js:246
> +                        var n = chCode - 0x30;

Is this problematic?  It overwrites they outer chCode.
Comment 4 Mark Lam 2020-01-12 15:34:00 PST
(In reply to Mark Lam from comment #3)
> Comment on attachment 387493 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387493&action=review
> 
> > Source/JavaScriptCore/builtins/RegExpPrototype.js:246
> > +                        var n = chCode - 0x30;
> 
> Is this problematic?  It overwrites they outer chCode.

Nevermind.  I misread that.  It sets n, not chCode.
Comment 5 Mark Lam 2020-01-12 15:59:29 PST
Comment on attachment 387494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387494&action=review

r=me if EWS bots are green.  Please undo the changes in the comments.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:495
> +    // 6. If flags contains "u", var unicodeMatching be true.
> +    // 7. Else, var unicodeMatching be false.

I think you should undo these.  The use of "let" here is in the English context, not the JS context.  Also, this is a mirror of the text in the spec.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:498
> +    // 8. If flags contains "y", var newFlags be flags.
> +    // 9. Else, var newFlags be the string that is the concatenation of flags and "y".

Ditto.  Please undo.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:513
> +    // 13. If limit is undefined, var lim be 2^32-1; else var lim be ? ToUint32(limit).

Ditto.  Undo.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:547
> +        // c. If z is null, var q be AdvanceStringIndex(S, q, unicodeMatching).

Ditto.  Undo.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:556
> +            // iii. If e = p, var q be AdvanceStringIndex(S, q, unicodeMatching).

Ditto.  Undo.
Comment 6 Yusuke Suzuki 2020-01-12 17:15:18 PST
Comment on attachment 387494 [details]
Patch

Thanks, fixed "let" => "var" changes in comments.
Comment 7 Yusuke Suzuki 2020-01-12 17:18:51 PST
Committed r254418: <https://trac.webkit.org/changeset/254418>
Comment 8 Radar WebKit Bug Importer 2020-01-12 17:19:14 PST
<rdar://problem/58516383>
Comment 9 Joseph Pecoraro 2020-01-13 11:45:12 PST
You may want to add a style checker or something, or lexical variables will probably start creeping in again.

Also, is there a bug for WebCore's builtins? A quick search found:
• Source/WebCore/Modules/streams