Bug 206321 – Pass JSToken by const reference

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jonathan Bedard]

Simon shared shared a lgtm link, this was one of issues identified.

The JSToken object is larger than the word size, so we should really be passing it by const reference, not by value.

https://lgtm.com/projects/g/WebKit/webkit/?tag=&lang=cpp&mode=tree&ruleFocus=2163210742

[Jonathan Bedard]

Created attachment 387859 [details]
Patch

[Saam Barati]

Comment on attachment 387859 [details]
Patch

This call site concerns me since m_token is a field that's updated as we parse more things

    pattern = createBindingPattern(context, kind, exportType, *m_token.m_data.ident, m_token, bindingContext, duplicateIdentifier);

[Saam Barati]

Comment on attachment 387859 [details]
Patch

Actually it looks like this function doesn’t lex, so we’re ok. Passing it by reference is ok since m_token won’t be updated

Can you verify also that it doesn’t end up lexing?

r=me

[Jonathan Bedard]

(In reply to Saam Barati from comment #3)
> Comment on attachment 387859 [details]
> Patch
> 
> Actually it looks like this function doesn’t lex, so we’re ok. Passing it by
> reference is ok since m_token won’t be updated
> 
> Can you verify also that it doesn’t end up lexing?
> 
> r=me

It does not lex. And reading through what the function is doing, seems like it shouldn't be lexing in the future either, although this change would sort of force that on us.

[WebKit Commit Bot]

Comment on attachment 387859 [details]
Patch

Clearing flags on attachment: 387859

Committed r254689: <https://trac.webkit.org/changeset/254689>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58648149>