Bug 159321 – Update JSC_functionOverrides to handle the new SourceCode strings that have params.

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

[Mark Lam]

And add tests so that this won't fail silently and bit rot anymore.  Patch coming.

[Mark Lam]

Created attachment 282487 [details]
proposed patch.

[Mark Lam]

Created attachment 282498 [details]
proposed patch 2: fixed EWS complaint.

[Geoffrey Garen]

Comment on attachment 282498 [details]
proposed patch 2: fixed EWS complaint.

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

> Source/JavaScriptCore/tools/FunctionOverrides.cpp:153
> +    String sourceString = origCode.view().toString();
> +    size_t sourceBodyStart = sourceString.find('{');
> +    if (sourceBodyStart == notFound)
> +        return false;
> +    String sourceBodyString = sourceString.substring(sourceBodyStart);
> +
> +    auto it = overrides.m_entries.find(sourceBodyString);

This is OK -- but I think it might be better to allow the author to override parameter declaration too.

[Mark Lam]

(In reply to comment #3)
> Comment on attachment 282498 [details]
> proposed patch 2: fixed EWS complaint.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282498&action=review
> 
> > Source/JavaScriptCore/tools/FunctionOverrides.cpp:153
> > +    String sourceString = origCode.view().toString();
> > +    size_t sourceBodyStart = sourceString.find('{');
> > +    if (sourceBodyStart == notFound)
> > +        return false;
> > +    String sourceBodyString = sourceString.substring(sourceBodyStart);
> > +
> > +    auto it = overrides.m_entries.find(sourceBodyString);
> 
> This is OK -- but I think it might be better to allow the author to override
> parameter declaration too.

I'll leave overriding parameters to another patch.  As far as I can tell, the uses of overriding parameters are:

1. Ability to use different argument names for better readability (especially if we're replacing an obfuscated function).
2. Ability to force stack arity adjustments.

I think (2) is the more meaningful reason for overriding parameters, but the need for that is probably rare.  I filed https://bugs.webkit.org/show_bug.cgi?id=159355 to track this for future work.

[Mark Lam]

Thanks for the review.  Landed in r202737: <http://trac.webkit.org/r202737>.