[webkit-reviews] review granted: [Bug 56041] RexExp constructor should only accept flags "gim" : [Attachment 85213] The patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 9 12:28:51 PST 2011


Darin Adler <darin at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 56041: RexExp constructor should only accept flags "gim"
https://bugs.webkit.org/show_bug.cgi?id=56041

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

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

> Source/JavaScriptCore/runtime/RegExp.h:27
> +#include "RegExpKey.h"

Seems a little awkward to have to include RegExpKey here to get this flags
enum.

>>> Source/JavaScriptCore/runtime/RegExp.h:40
>>> +	     static PassRefPtr<RegExp> create(JSGlobalData* globalData, const
UString& pattern, RegExpFlags flags);
>> 
>> The parameter name "globalData" adds no information, so it should be
removed.  [readability/parameter_name] [5]
> 
> The parameter name "flags" adds no information, so it should be removed. 
[readability/parameter_name] [5]

I agree with the stylebot.

>>> Source/JavaScriptCore/runtime/RegExp.h:60
>>> +	     RegExp(JSGlobalData* globalData, const UString& pattern,
RegExpFlags flags);
>> 
>> The parameter name "globalData" adds no information, so it should be
removed.  [readability/parameter_name] [5]
> 
> The parameter name "flags" adds no information, so it should be removed. 
[readability/parameter_name] [5]

I agree with the stylebot.

>> Source/JavaScriptCore/runtime/RegExpCache.h:44
>> +	PassRefPtr<RegExp> lookupOrCreate(const UString& patternString,
RegExpFlags flags);
> 
> The parameter name "flags" adds no information, so it should be removed. 
[readability/parameter_name] [5]

I agree with the stylebot.

>>> Source/JavaScriptCore/runtime/RegExpCache.h:45
>>> +	 PassRefPtr<RegExp> create(const UString& patternString, RegExpFlags
flags, RegExpCacheMap::iterator iterator);
>> 
>> The parameter name "flags" adds no information, so it should be removed. 
[readability/parameter_name] [5]
> 
> The parameter name "iterator" adds no information, so it should be removed. 
[readability/parameter_name] [5]

I agree with the stylebot.

> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:313
> +	   flags = regExpFlags(arg1.toString(exec));

Should return here if toString raised an exception rather than continuing with
NoFlags.

> Source/JavaScriptCore/runtime/RegExpKey.h:36
> +enum RegExpFlags {

Flag bits are not quite an enum since they can be enum values or'ed together.
In some places we’ve used a separate integral typedef for this. I guess this is
OK.

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:98
> +	       flags = regExpFlags(arg1.toString(exec));

Should return here if toString raised an exception rather than continuing with
NoFlags.


More information about the webkit-reviews mailing list