Add support for VS2013 #65

kfarnung · 2017-06-22T23:48:44Z

VS2013 issues

No constexpr support
- Conditionally use constexpr if the compiler supports it
- Updated tests to use a macro for this purpose
Requires copy constructor support for base classes of thrown objects
- Added a protected copy constructor to ObjectReference and
  Reference
Incorrect overload resolution for std::initializer_list
- Used explicit types to invoke the correct overload
No literal support for char16_t
- Added macro to convert the text properly
Lambdas are unable to capture function non-pointers
- Updated the typedef to create a function pointer
Short lifetime for temporary objects passed to constructors
- Updated the tests to create the objects and pass them to the
  constructor. This is closer to how they would be used anyway

General improvements

Added console.log output to the test runner to make it more clear
when and where tests are failing

kfarnung · 2017-06-22T23:48:59Z

kfarnung · 2017-06-22T23:49:43Z

I'm still investigating test failures that I'm seeing in Travis with node-chakracore, but all other legs complete successfully.

jasongin · 2017-06-22T23:51:31Z

napi.h

+#define NAPI_HAS_CONSTEXPR 1
+#endif
+
+// VS2015 RTM has bugs with constexpr, so require min of VS2015 Update 3 (known good version)


Change this comment to reference the char16_t literal issue.

jasongin · 2017-06-22T23:56:03Z

test/function.cc

@@ -62,7 +62,7 @@ Value CallWithVector(const CallbackInfo& info) {
 Value CallWithReceiverAndArgs(const CallbackInfo& info) {
   Function func = info[0].As<Function>();
   Value receiver = info[1];
-   return func.Call(receiver, { info[2], info[3], info[4] });
+   return func.Call(receiver, { static_cast<napi_value>(info[2]), static_cast<napi_value>(info[3]), static_cast<napi_value>(info[4]) });


nit: Wrap lines at 100 cols.
(There is no linter set up yet...)

Are there specific guidelines for how to wrap lines? I looked through the existing code and didn't see enough forced wraps to draw solid conclusions.

No specific guidelines.

jasongin · 2017-06-23T00:00:09Z

napi-inl.h

+    napi_status status = napi_create_reference(_env, value, 1, &_ref);
+
+    // TODO - What's the right way to handle errors in the constructor without
+    //        being able to throw exceptions?


napi_fatal_error()... but it doesn't exist yet. :)

jasongin · 2017-06-23T00:02:46Z

napi-inl.h

+
+  napi_value value = other.Value();
+  if (value != nullptr) {
+    napi_status status = napi_create_reference(_env, value, 1, &_ref);


This is always creating a strong reference, even if other was a weak reference. I guess it's OK since this should only ever be called by the Error class which is always strong. At least add a comment explaining that is the purpose of this constructor.

jasongin

LGTM, assuming tests pass on all targeted compilers/platforms.

kfarnung · 2017-06-23T03:26:51Z

@jasongin The CI is clean now, but I'm running into a test failure with VS2013 (now that the tests actually run).

I'll pick this up again tomorrow and figure out why the test is failing.

> node test

Starting test suite

Running test 'arraybuffer'
Running test 'asyncworker'
Running test 'buffer'
Running test 'error'
Running test 'external'
Running test 'function'
Running test 'name'
Running test 'object'
E:\GitHub\node-addon-api\test\object.js:23
    binding.object.defineProperties(obj, nameType);
                   ^

Error: Invalid pointer passed as argument
    at testDefineProperties (E:\GitHub\node-addon-api\test\object.js:23:20)
    at test (E:\GitHub\node-addon-api\test\object.js:64:3)
    at Object.<anonymous> (E:\GitHub\node-addon-api\test\object.js:5:1)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Module.require (module.js:513:17)
    at require (internal/module.js:11:18)

EDIT: It turns out that VS2013 has some behavioral differences from the other compilers with regard to temporary objects created in constructor calls. In this case we take the string, extract the str.c_str() from it, then return. When the constructor ends the std::string is destroyed and the str.c_str() memory is invalidated. The "correct" behavior from a C++ perspective would be to copy or move the std::string into the object so that the lifetime is owned by the new object, but I don't think that the scenario is common enough to warrant that. It's also appears (just through successful test runs) that more modern compilers modify the lifetime behavior to at least survive the top-level function call.

kfarnung · 2017-06-23T22:55:35Z

@jasongin @boingoing I have resolved all build and test issues, is there anything else I should be testing with?

VS2013 issues * No constexpr support - Conditionally use constexpr if the compiler supports it - Updated tests to use a macro for this purpose * Requires copy constructor support for base classes of thrown objects - Added a protected copy constructor to ObjectReference and Reference<T> * Incorrect overload resolution for std::initializer_list - Used explicit types to invoke the correct overload * No literal support for char16_t - Added macro to convert the text properly * Lambdas are unable to capture function non-pointers - Updated the typedef to create a function pointer * Short lifetime for temporary objects passed to constructors - Updated the tests to create the objects and pass them to the constructor. This is closer to how they would be used anyway General improvements * Added console.log output to the test runner to make it more clear when and where tests are failing

kfarnung · 2017-06-24T00:18:26Z

Everything is passing now locally building in VS2013 and VS2015 and the CI is green for all legs.

jasongin · 2017-06-24T05:25:18Z

test/object.cc

-      PropertyDescriptor::Value(std::string("enumerableValue"), trueValue, napi_enumerable),
-      PropertyDescriptor::Value(std::string("configurableValue"), trueValue, napi_configurable),
-      PropertyDescriptor::Function(std::string("function"), TestFunction),
+      PropertyDescriptor::Accessor(str1, TestGetter),


This is unfortunate, and seems likely to confuse many people (if they are using VS 2013). The way I wanted this to work for the most common case is you should be able to just pass in a string literal:

PropertyDescriptor::Accessor("readonlyAccessor", TestGetter),

Did you try that? I'd assume the behavior is the same as the explicit call to std::string(), but I'm not sure.

If that doesn't work, then I think it would be best to add overloads of each of those methods that accept a const char* instead.

@jasongin there is already an overload for const char *, that's what the other leg of the conditional tests. It just gets chained to for the std::string constructor. Does that resolve your concerns?

Oh, sorry I didn't look at the broader context in this file! Yes, that resolves my concerns then.

jasongin

I think we need to do something better for the temporary std::string issue.

boingoing

👍

boingoing · 2017-06-23T00:44:03Z

napi-inl.h

@@ -2697,11 +2716,11 @@ inline FunctionReference& AsyncWorker::Callback() {
 }

 inline void AsyncWorker::OnOK() {
-  _callback.MakeCallback(_receiver.Value(), {});
+  _callback.MakeCallback(_receiver.Value(), {{}});


This double-curly syntax is weird. Can you add a comment?

This should have been changed in the latest revision. This actually had side-effects that I encountered during testing.

kfarnung · 2017-06-26T19:01:09Z

@jasongin As far as I'm concerned this is ready to land, feel free to land it at your convenience.

jasongin reviewed Jun 22, 2017

View reviewed changes

jasongin reviewed Jun 23, 2017

View reviewed changes

kfarnung mentioned this pull request Jun 23, 2017

VS 2013 Support #58

Closed

jasongin approved these changes Jun 23, 2017

View reviewed changes

kfarnung changed the title ~~Allow usage in VS2013~~ Add support for VS2013 Jun 23, 2017

kfarnung force-pushed the vs2013 branch from f10fa70 to ae2c05f Compare June 23, 2017 23:49

jasongin reviewed Jun 24, 2017

View reviewed changes

jasongin suggested changes Jun 24, 2017

View reviewed changes

jasongin approved these changes Jun 24, 2017

View reviewed changes

boingoing approved these changes Jun 26, 2017

View reviewed changes

jasongin merged commit 617dbb6 into nodejs:master Jun 26, 2017

kfarnung deleted the vs2013 branch June 26, 2017 21:28

JckXia mentioned this pull request Apr 30, 2021

[Tests] Add test coverage for PropertyDescriptors #986

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for VS2013 #65

Add support for VS2013 #65

kfarnung commented Jun 22, 2017 •

edited

Loading

kfarnung commented Jun 22, 2017

kfarnung commented Jun 22, 2017

jasongin Jun 22, 2017

jasongin Jun 22, 2017

kfarnung Jun 23, 2017

jasongin Jun 23, 2017

jasongin Jun 23, 2017

jasongin Jun 23, 2017

jasongin left a comment

kfarnung commented Jun 23, 2017 •

edited

Loading

kfarnung commented Jun 23, 2017

kfarnung commented Jun 24, 2017

jasongin Jun 24, 2017

kfarnung Jun 24, 2017

jasongin Jun 24, 2017

jasongin left a comment

boingoing left a comment

boingoing Jun 23, 2017

kfarnung Jun 26, 2017

kfarnung commented Jun 26, 2017

Add support for VS2013 #65

Add support for VS2013 #65

Conversation

kfarnung commented Jun 22, 2017 • edited Loading

kfarnung commented Jun 22, 2017

kfarnung commented Jun 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasongin left a comment

Choose a reason for hiding this comment

kfarnung commented Jun 23, 2017 • edited Loading

kfarnung commented Jun 23, 2017

kfarnung commented Jun 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasongin left a comment

Choose a reason for hiding this comment

boingoing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung commented Jun 26, 2017

kfarnung commented Jun 22, 2017 •

edited

Loading

kfarnung commented Jun 23, 2017 •

edited

Loading