[webkit-reviews] review denied: [Bug 45959] String.prototype.replace passes undefined rather than empty string for groups that don't match : [Attachment 68002] Proposed patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Mark Hahnenberg
<mhahnenb at gmail.com>'s request for review:
Bug 45959: String.prototype.replace passes undefined rather than empty string
for groups that don't match
https://bugs.webkit.org/show_bug.cgi?id=45959

Attachment 68002: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=68002&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
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));