[webkit-reviews] review denied: [Bug 50554] RegExp.prototype.toString does not escape slashes : [Attachment 97691] The patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 50554: RegExp.prototype.toString does not escape slashes
https://bugs.webkit.org/show_bug.cgi?id=50554

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=97691&action=review

Looks like this caused some test failures"

  jquery/manipulation.html = TIMEOUT

Regressions: Unexpected text diff mismatch : (1)
  fast/regex/toString.html = TEXT

Could you check into those?

> Source/JavaScriptCore/runtime/RegExpObject.cpp:117
> +    size_t fwdSlash = pattern.find('/');

This local variable does not contain a forward slash. I think a better name
would be forwardSlashLocation or forwardSlashPosition.

> Source/JavaScriptCore/runtime/RegExpObject.cpp:129
> +	   size_t slashes = fwdSlash;

I would call this slashesPosition instead of slashes. Again, it's a number, not
slashes, which is why.

>> Source/JavaScriptCore/runtime/RegExpObject.cpp:130
>> +	    while(slashes && pattern[slashes - 1] == '\\')
> 
> Missing space before ( in while(  [whitespace/parens] [5]

The style bot is right about this missing space.

> Source/JavaScriptCore/runtime/RegExpObject.cpp:135
> +	   // doubel escape it.

Typo: doubel

> Source/JavaScriptCore/runtime/RegExpObject.cpp:137
> +	       result.append(pattern.substringSharingImpl(completed, fwdSlash +
1));

This tells me that substringSharingImpl needs a better name; we need a name
that makes it clearer when you need to use it and when you must not use it.

> LayoutTests/fast/regex/script-tests/toString.js:11
> +    shouldBeTrue("re1.test(string)");
> +    shouldBeTrue("re2.test(string)");

Can you write this so that the patterns are visible in the shouldBe
expressions, making the test output clearer? I think you should.

> LayoutTests/fast/regex/script-tests/toString.js:22
> +// These strings are equilvalent, since the '\' is identity escaping the '/'
at the string level.

Typo: equivalent