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

Can not use IDL long type as return type. #120

Open
romandev opened this issue Oct 8, 2017 · 4 comments
Open

Can not use IDL long type as return type. #120

romandev opened this issue Oct 8, 2017 · 4 comments

Comments

@romandev
Copy link
Member

romandev commented Oct 8, 2017

In the current implementation, we can not use IDL long type as return type. If we add some test method returning long type, the following build error occurs.

.././core/js_type_traits.h:25:10: error: no viable conversion from returned value of type 'long' to function return type 'Napi::Value'
  return T();
         ^~~
Release/obj/gen/test/test_interface_bridge.cc:286:10: note: in instantiation of function template specialization 'JSTypeTraits<long>' requested here
  return JSTypeTraits(info.Env(), return_value);
         ^
/Users/zino/bacardi/node_modules/node-addon-api/napi.h:118:9: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'long' to 'const Napi::Value &' for 1st argument
  class Value {
        ^
/Users/zino/bacardi/node_modules/node-addon-api/napi.h:118:9: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'long' to 'Napi::Value &&' for 1st argument
1 error generated.

The above error is because there is no template-specialization for long type in JSTypeTraits.
So, if we add the specialization, it works. But it's not a right way I think.
The long type's range might be changed in C++ side depending on each platform. So, we should find a better way to map explicit range type such as int32_t.

@romandev
Copy link
Member Author

romandev commented Oct 8, 2017

@corona10 Are u interested in this issue?

@corona10
Copy link
Contributor

corona10 commented Oct 8, 2017

Sure!

corona10 added a commit to corona10/bacardi that referenced this issue Oct 8, 2017
romandev pushed a commit that referenced this issue Oct 8, 2017
Generate JSTypeTraits by macro functions

In the future, we can auto-generate js_type_traits during bacardi build
time by using some information of types with template.

e.g
types = {
  'Number': {
    int32_t,
    int64_t,
    float,
    long
    .
    .
  },
  'String': {
     char*,
     std::string,
     .
   }
  
}
And this is the just pseudo code for concept.

if type in types['Number']:
   generate("JS_TYPE_TRAITS_NUMBER("+type+")");

if type in types['String']:
   generate("JS_TYPE_TRAITS_STRING("+type+")");

ISSUE=#120
corona10 added a commit to corona10/bacardi that referenced this issue Oct 8, 2017
By doing this we can easily generate various basic types.

ISSUE=lunchclass#125, lunchclass#120
corona10 added a commit to corona10/bacardi that referenced this issue Oct 8, 2017
By doing this we can easily generate various basic types.

ISSUE=lunchclass#125, lunchclass#120
romandev pushed a commit that referenced this issue Oct 18, 2017
Generate JSTypeTraits by macro functions

In the future, we can auto-generate js_type_traits during bacardi build
time by using some information of types with template.

e.g
types = {
  'Number': {
    int32_t,
    int64_t,
    float,
    long
    .
    .
  },
  'String': {
     char*,
     std::string,
     .
   }
  
}
And this is the just pseudo code for concept.

if type in types['Number']:
   generate("JS_TYPE_TRAITS_NUMBER("+type+")");

if type in types['String']:
   generate("JS_TYPE_TRAITS_STRING("+type+")");

ISSUE=#120
@romandev
Copy link
Member Author

Are you working on this? If not so, I can take this issue.

@corona10
Copy link
Contributor

@romandev It might be better.

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

No branches or pull requests

2 participants