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
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

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