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

Add missing methods to Nan::Maybe<T> #852

Merged
merged 2 commits into from May 16, 2019

Conversation

@bnoordhuis
Copy link
Member

commented May 1, 2019

Nan::Maybe was an alias for v8::Maybe with most Node.js versions
but that's problematic because v8::Maybe has grown new methods over
time that are missing in older versions.

From now on we use our custom Maybe implementation with all Node.js
versions. This commit also adds the following missing methods:

  • Maybe::Check() const
  • Maybe::To(T*) const
  • Maybe::ToChecked() const

Fixes: #851

@LinusU
LinusU approved these changes May 1, 2019
Copy link
Contributor

left a comment

Awesome! Thanks for the quick turn around on this 😍

@bnoordhuis bnoordhuis requested a review from kkoopa May 6, 2019
@bnoordhuis bnoordhuis force-pushed the bnoordhuis:fix851 branch from 4cceeda to 1a6c33e May 6, 2019
@kkoopa
kkoopa approved these changes May 8, 2019
Copy link
Collaborator

left a comment

LGTM with a small nit

nan.h Outdated
#if defined(V8_MAJOR_VERSION) && (V8_MAJOR_VERSION > 4 || \
(V8_MAJOR_VERSION == 4 && defined(V8_MINOR_VERSION) && V8_MINOR_VERSION >= 3))
// Allow implicit conversions from v8::Maybe<T> to Nan::Maybe<T>.
Maybe(const v8::Maybe<T>& that)

This comment has been minimized.

Copy link
@kkoopa

kkoopa May 8, 2019

Collaborator

can you add a comment for the linter to suppress warnings about runtime/explicit here

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 16, 2019

Author Member

Done, sorry about that. I added a second commit that fixes all the other style violations.

bnoordhuis added 2 commits May 1, 2019
Nan::Maybe<T> was an alias for v8::Maybe<T> with most Node.js versions
but that's problematic because v8::Maybe<T> has grown new methods over
time that are missing in older versions.

From now on we use our custom Maybe<T> implementation with all Node.js
versions. This commit also adds the following missing methods:

* Maybe<T>::Check() const
* Maybe<T>::To(T*) const
* Maybe<T>::ToChecked() const

Fixes: #851
The code base is now `make lint`-clean again.
@bnoordhuis bnoordhuis force-pushed the bnoordhuis:fix851 branch from 1a6c33e to d1131c7 May 16, 2019
@kkoopa

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

Even better. Thanks.

@kkoopa kkoopa merged commit 4e96248 into nodejs:master May 16, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

@kkoopa Is there a reason you squashed the commits? Not that it really matters, I'm just curious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.