API: expose RegExp.

API: expose RegExp.

Committed: http://code.google.com/p/v8/source/detail?r=5584

[antonm, 2010-10-04 14:44:55]

LGTM

http://codereview.chromium.org/3585010/diff/1/3
File src/api.cc (right):

http://codereview.chromium.org/3585010/diff/1/3#newcode3730
src/api.cc:3730: ASSERT(num_flags <= static_cast<int>(sizeof(flags_buf)));
You might prefer to use ARRAY_SIZE macro here

http://codereview.chromium.org/3585010/diff/1/3#newcode3731
src/api.cc:3731: if (num_flags == 0) return i::Factory::empty_string();
should we have this special case here?  if it's performance critical, maybe do
the check at the top of the function?

http://codereview.chromium.org/3585010/diff/1/3#newcode3753
src/api.cc:3753: if (!obj->IsJSRegExp()) return Local<v8::RegExp>();
could this condition be true?

http://codereview.chromium.org/3585010/diff/1/5
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/3585010/diff/1/5#newcode11628
test/cctest/test-api.cc:11628: v8::Handle<v8::RegExp> re =
v8::RegExp::New(v8_str("foo"), v8::RegExp::kNone);
maybe add a test case that checks that regexps created via API can function like
normal regexps

[Vitaly Repeshko, 2010-10-04 15:04:41]

Thanks for speedy review! Submitted.


-- Vitaly

http://codereview.chromium.org/3585010/diff/1/3
File src/api.cc (right):

http://codereview.chromium.org/3585010/diff/1/3#newcode3730
src/api.cc:3730: ASSERT(num_flags <= static_cast<int>(sizeof(flags_buf)));
On 2010/10/04 14:44:55, antonm wrote:
> You might prefer to use ARRAY_SIZE macro here

Done. But it's longer and still requires a cast to int.

http://codereview.chromium.org/3585010/diff/1/3#newcode3731
src/api.cc:3731: if (num_flags == 0) return i::Factory::empty_string();
On 2010/10/04 14:44:55, antonm wrote:
> should we have this special case here?  if it's performance critical, maybe do
> the check at the top of the function?

Changed to Factory::LookupSymbol.

http://codereview.chromium.org/3585010/diff/1/3#newcode3753
src/api.cc:3753: if (!obj->IsJSRegExp()) return Local<v8::RegExp>();
On 2010/10/04 14:44:55, antonm wrote:
> could this condition be true?

With the current spec only if there's a bug in regexp.js. Removed.

http://codereview.chromium.org/3585010/diff/1/5
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/3585010/diff/1/5#newcode11628
test/cctest/test-api.cc:11628: v8::Handle<v8::RegExp> re =
v8::RegExp::New(v8_str("foo"), v8::RegExp::kNone);
On 2010/10/04 14:44:55, antonm wrote:
> maybe add a test case that checks that regexps created via API can function
like
> normal regexps

Done.

[Mads Ager (chromium), 2010-10-05 08:55:46]

LGTM. I think the implementation can be simplified a bit.

http://codereview.chromium.org/3585010/diff/1/2
File include/v8.h (right):

http://codereview.chromium.org/3585010/diff/1/2#newcode1374
include/v8.h:1374: V8EXPORT static Local<RegExp> New(Handle<String> pattern,
Could you add comments to this method as well. You should add an example of
using flags.

http://codereview.chromium.org/3585010/diff/1/2#newcode1383
include/v8.h:1383: V8EXPORT Flags GetFlags() const;
Could you add comments to this method as well?

http://codereview.chromium.org/3585010/diff/1/3
File src/api.cc (right):

http://codereview.chromium.org/3585010/diff/1/3#newcode3748
src/api.cc:3748: i::Handle<i::Object> obj = i::Execution::New(
I think this code can be simplified by creating a Factory::NewJSRegExp method to
call instead of going into JavaScript code here. I believe the NewJSRegExp
function could be something like this:

Handle<JSRegExp> Factory::NewJSRegExp(Handle<String> pattern,
                                      Handle<String> flags) {
  Handle<JSFunction> constructor(Top::regexp_function());
  bool has_pending_exception;
  Handle<Object> regexp =
      RegExpImpl::CreateRegExpLiteral(constructor,
                                      pattern,
                                      flags,
                                      &has_pending_exception);
  if (has_pending_exception) return Handle<JSRegExp>::null();
  return Handle<JSRegExp>::cast(regexp);
}

Then this code could be:

i::Handle<i::JSRegExp> obj = i::Factory::NewJSRegExp(pattern, flags);
return scope.Close(Utils::ToLocal(obj));

http://codereview.chromium.org/3585010/diff/1/3#newcode3766
src/api.cc:3766: #define REGEXP_FLAG_ASSERT_EQ(api_flag, internal_flag)        \
Is this really worth the macro. Writing this out wouldn't take much more space
and would be just as readable I think?

[Vitaly Repeshko, 2010-10-05 10:58:00]

Mads,

Thanks for the additional comments. I'm addressing them in the patch I just sent
you.


-- Vitaly