Skip to content
New issue

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

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

Already on GitHub? # to your account

Fixed deprecated usage of v8::String::Value constructor #91

Closed
wants to merge 1 commit into from

Conversation

atvise
Copy link

@atvise atvise commented Sep 7, 2018

  • v8 has declared the usage of the v8::String::Value constructor without isolate parameter as deprecated therefore add already available isolate as parameter

@atvise atvise force-pushed the fix_deprecated_usage branch from 270bb35 to b808c76 Compare September 7, 2018 07:40
@pmed
Copy link
Owner

pmed commented Sep 15, 2018

Hi,

Thank you for the contribution to the project.

There are other deprecated V8 API parts could be used in the library, since I try to support older V8 versions (at least from version 6.0)

I would prefer to stay with this deprecation yet some time, because there is no such a constructor with isolate in V8 version 6.2.

@atvise
Copy link
Author

atvise commented Sep 18, 2018

Your welcome!

My suggestion would be a version define switch for such instances. Would that be ok with you ?

Like:

#if V8_MAJOR_VERSION == 6 && V8_MINOR_VERSION <= 0
// code for <= 6.0
#else
// code for >= 6.1 
#endif

pmed added a commit that referenced this pull request Sep 21, 2018
@pmed
Copy link
Owner

pmed commented Sep 21, 2018

I've added isolate argument for v8::String ctors in other places: examples, test.

In V8 version 7.0 this deprecated ctor without isolate was removed. Honestly, I don't want to pollute the code base with #ifdef conditions. Seems that's time to move on, so I'm going to release a new v8pp version with V8 support starting from 6.3.

- v8 has declared the usage of the v8::String::Value constructor without isolate parameter as deprecated therefore add already available isolate as parameter
@atvise atvise force-pushed the fix_deprecated_usage branch from b808c76 to 8de80ef Compare October 1, 2018 12:02
@pmed
Copy link
Owner

pmed commented Oct 19, 2018

Hi @atvise,

Thank you, I've manually merged this PR in db0f110

@pmed pmed closed this Oct 19, 2018
@atvise
Copy link
Author

atvise commented Oct 22, 2018

Thx !

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants