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

Linted code #581

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Linted code #581

wants to merge 7 commits into from

Conversation

kkoopa
Copy link
Collaborator

@kkoopa kkoopa commented Jun 12, 2016

@bnoordhuis
Copy link
Member

The explicitness vs. implicitness of constructors seems a bit random. What's the pattern?

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jun 13, 2016

V8 has MaybeLocal do conversion. I was a bit befuddled regarding Persistents, but tried to stick to that of V8 3.19 or so, which was the last one with single argument constructors. Some of them were explicit, others were not.

On June 13, 2016 9:10:52 AM GMT+03:00, Ben Noordhuis [email protected] wrote:

The explicitness vs. implicitness of constructors seems a bit random.
What's the pattern?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#581 (comment)

@bnoordhuis
Copy link
Member

Most things are explicit now and personally I think that's better, particularly with persistent handles, because there is less opportunity for accidental memory leaks. Possibly less of an issue with v4 and newer than it is with v0.10 and v0.12 because v8::Persistent is by default non-copyable.

MaybeLocal, like you point out, is the odd duck out for some reason.

v8::Persistent<T, M>(v8::Isolate::GetCurrent(), that) {}

template<typename S, typename M2>
inline Persistent(const v8::Persistent<S, M2> &that) :
inline
Persistent(const v8::Persistent<S, M2> &that) : // NOLINT(runtime/explicit)
v8::Persistent<T, M2>(v8::Isolate::GetCurrent(), that) {}

This comment was marked as off-topic.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jun 14, 2016

There, turned out there were rather many wrong things which had been masked by implicit conversion. Now it should be sorted out.

@@ -14,25 +14,31 @@ template<typename T, typename M> class Persistent :
public:
inline Persistent() : v8::Persistent<T, M>() {}

template<typename S> inline Persistent(v8::Local<S> that) :
template<typename S> inline explicit Persistent(v8::Local<S> that) :

This comment was marked as off-topic.

@bnoordhuis
Copy link
Member

I think I understand why the code is doing what it's doing but it's really skirting UB territory now, isn't it?

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jun 15, 2016

Skirting, but never crossing. Nothing is virtual and no child has added any data members. Additionally, the horrible things only occur for old versions, which will not change retroactively.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jun 15, 2016

I changed the casts to static_cast and added some tests to help put your mind at ease.

# else
inline MaybeLocal(v8::Local<S> that) :
inline MaybeLocal(v8::Local<S> that) : // NOLINT(runtime/explicit)
val_(*reinterpret_cast<v8::Local<T>*>(&that)) {}

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@bnoordhuis
Copy link
Member

LGTM

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jun 16, 2016

Thanks for looking through this.

I did find an edge case in that it is now not reasonably possible to actually copy a Global from another Global. It will require a static_cast from Global to PersistentBase, which is presumably unavoidable and can be lived with. However, due to the way things are set up, that does not work either, since Global inherits from v8::PersistentBase but not from Nan::PersistentBase.

The best solution ought to be the normalization process from #492, which would set up the correct inheritance hierarchy, remove some code duplication and make everything better in the long run. I will open a new PR with these commits plus the additional ones.

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

Successfully merging this pull request may close these issues.

2 participants