[Webkit-unassigned] [Bug 45959] String.prototype.replace passes undefined rather than empty string for groups that don't match

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45959


Oliver Hunt <oliver at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68002|review?                     |review-
               Flag|                            |




--- Comment #3 from Oliver Hunt <oliver at apple.com>  2010-09-18 09:38:49 PST ---
(From update of attachment 68002)
View in context: https://bugs.webkit.org/attachment.cgi?id=68002&action=prettypatch

Hi mark,
The logic of the patch is correct, only the style issues i mentioned stand in the way of that.

Before we can land the patch though we'll need to have a testcase for this new behaviour (so we don't regress).  I'm actually surprised we don't already have a test for this (checking for the wrong behaviour) have you run the full webkit test suite (it's somewhat unfortunate but the majority of our js tests are part of the webkit test harness these days so require a full build of webkit :-/)

Assuming you've built webkit using build-webkit you should be able to do run-webkit-tests fast/js to run the bulk of the js tests (which will show whether any existing test covers this case).  If a test already covers this but expects the wrong behaviour, just correct the test to check for the right thing.  Otherwise just add a few tests to the end of LayoutTests/fast/js/script-tests/string-replace-3.js and regenerate the expected output.

> JavaScriptCore/runtime/StringPrototype.cpp:296
> +    JSGlobalData* globalData;

Don't declare globalData here, as you're not initialising it, and it's only used in one placed anyway

> JavaScriptCore/runtime/StringPrototype.cpp:344
> +                        globalData = &exec->globalData();

Make this globalData actually be the declaration.

It is worth considering not even having the temporary -- i'm not sure it really gains anything over simply having
cachedCall.setArgument(i, exec->globalData().smallStrings.emptyString(globalData));

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.