Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: workaround for possible V8 bug fixes #4266 #4299

Closed
wants to merge 1 commit into from

Conversation

skilledDeveloper
Copy link

workaround for possible V8 bug (due to the use of .ToHandleChecked() in deps/v8/src/api.cc)
Original issue: #4266
@bnoordhuis please review

workaround for possible V8 bug (due to the use of .ToHandleChecked() in deps/v8/src/api.cc)
@bnoordhuis please review
@silverwind silverwind added the v8 engine Issues and PRs related to the V8 dependency. label Dec 15, 2015
@bnoordhuis
Copy link
Member

A couple of comments:

  1. This PR should be against the v0.12-staging branch.
  2. Don't use magic constants, add a static const size_t kMaxLength = ... to e.g. ExternString.
  3. There is no point in creating the ExternString if you're not going to use it.
  4. Returning an empty string is the wrong thing to do. Returning an empty handle may be okay.
  5. Comment style: // Capitalized and punctuated.

@bnoordhuis
Copy link
Member

Forgot to mention: please add regression tests.

@skilledDeveloper
Copy link
Author

OK. I'll create a new PR for this.

@skilledDeveloper
Copy link
Author

@bnoordhuis
Does a test like this work?

@bnoordhuis
Copy link
Member

Yes, you're welcome to steal tests from the master branch. They'll probably need some adjusting though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants