Improved compatiblity with newer node versions > 10 and electron > 4.0.0 #2
Conversation
There was a problem hiding this comment.
Thanks, I don't have any real remarks about these changes, they're probably fine, but there are still a couple of points I'd like to address:
- I'm a bit confused by
LocalChecked()use, it seems to exist in earlier Node API versions too, yet we only use it with 8+, why? Shouldn't we just always use it? - Commit messages are really not great. They should say things like "Removed MarkIndependent() calls for Node v8+ as they're deprecated and do nothing in the Node API now" or "Added SWIGV8_CURRENT_CONTEXT() calls for Node v8+ as ..." etc. Also maybe I wouldn't even have a question about
ToLocalChecked()if this change were explained in the commit message.
| #define SWIGV8_ADJUST_MEMORY(size) v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(size) | ||
| #define SWIGV8_CURRENT_CONTEXT() v8::Isolate::GetCurrent()->GetCurrentContext() | ||
| #define SWIGV8_THROW_EXCEPTION(err) v8::Isolate::GetCurrent()->ThrowException(err) |
There was a problem hiding this comment.
Those don't really seem different? Or am I missing something?
| @@ -47,12 +49,18 @@ typedef v8::PropertyCallbackInfo<v8::Value> SwigV8PropertyCallbackInfo; | |||
| #define SWIGV8_THROW_EXCEPTION(err) v8::ThrowException(err) | |||
| #define SWIGV8_STRING_NEW(str) v8::String::New(str) | |||
| #define SWIGV8_SYMBOL_NEW(sym) v8::String::NewSymbol(sym) | |||
| #else | |||
| #elif ((V8_MAJOR_VERSION-0) < 7 && (V8_MINOR_VERSION-0) <= 9) || ((V8_MAJOR_VERSION-0) == 7 && (V8_MINOR_VERSION-0) < 9) | |||
There was a problem hiding this comment.
This check doesn't make sense to me, was it supposed to be
| #elif ((V8_MAJOR_VERSION-0) < 7 && (V8_MINOR_VERSION-0) <= 9) || ((V8_MAJOR_VERSION-0) == 7 && (V8_MINOR_VERSION-0) < 9) | |
| #elif ((V8_MAJOR_VERSION-0) < 7) || ((V8_MAJOR_VERSION-0) == 7 && (V8_MINOR_VERSION-0) < 9) |
?
I must also say that all those -0 are annoying, even if I understand that they're used in the existing code. IMHO adding a macro SWIG_CHECK_V8_VERSION() that could be then used as
#if !SWIG_CHECK_V8_VERSION(7, 9)
could be worth it.
| #else | ||
| #define SWIGV8_TO_OBJECT(handle) (handle)->ToObject(SWIGV8_CURRENT_CONTEXT()).ToLocalChecked() | ||
| #define SWIGV8_TO_STRING(handle) (handle)->ToString(SWIGV8_CURRENT_CONTEXT()).ToLocalChecked() | ||
| #define SWIGV8_NUMBER_VALUE(handle) (handle)->NumberValue(SWIGV8_CURRENT_CONTEXT()).ToChecked() | ||
| #define SWIGV8_INTEGER_VALUE(handle) (handle)->IntegerValue(SWIGV8_CURRENT_CONTEXT()).ToChecked() | ||
| #define SWIGV8_BOOLEAN_VALUE(handle) (handle)->BooleanValue(v8::Isolate::GetCurrent()) | ||
| #define SWIGV8_WRITE_UTF8(handle, buffer, len) (handle)->WriteUtf8(v8::Isolate::GetCurrent(), buffer, len) | ||
| #define SWIGV8_UTF8_LENGTH(handle) (handle)->Utf8Length(v8::Isolate::GetCurrent()) |
There was a problem hiding this comment.
I also don't see any real differences here...
| if (ptr == NULL) { | ||
| #if (V8_MAJOR_VERSION-0) < 4 && (SWIG_V8_VERSION < 0x031903) | ||
| SWIGV8_ESCAPE(SWIGV8_NULL()); | ||
| #else | ||
| #else |
There was a problem hiding this comment.
Please avoid making whitespace-only changes in the same commits as significant ones.
| @@ -6,6 +6,8 @@ | |||
|
|
|||
| %include <std_common.i> | |||
|
|
|||
| %include <std_vector.i> | |||
There was a problem hiding this comment.
Changes in this file seem to be unrelated and part of a different PR.
|
BTW, have you seen swig#1702? It seems to overlap with this PR in goals, but does something rather different... |
I did and forked is repo, however I just saw I worked with his master and not with the proper nodejs-v12-master branch. :-( Even if I tested some time ago with is branch and had some issues. What do you think I should do? keep my PR since it is working with opscore? Execute the tests based on is Alexander fork ? |
|
I didn't look at the details of their changes, so I don't really know... Do we need them too? If not, why? Of course, there is also the converse question: how did they manage to make things work without your changes? But this is probably less important. The end goal is to make it possible to merge your changes into the upstream SWIG version and for this it should either be compatible with the other PR (in the sense that it shouldn't result in any merge conflicts and that things should actually work when both changes are applied), or should replace it completely. If we make incompatible changes in our fork, we risk having a lot of trouble reconciling them with the upstream SWIG in the future, so it would be best to avoid this. |
expand testcase to test for new casting behavior
Update octruntime.swg to work with Octave v8.1.0
No description provided.