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

Generate JSTypeTraits automatically. #127

Closed
wants to merge 1 commit into from

Conversation

corona10
Copy link
Contributor

@corona10 corona10 commented Oct 8, 2017

By doing this, we can easily generate various basic types.

ISSUE=#125, #120

By doing this we can easily generate various basic types.

ISSUE=lunchclass#125, lunchclass#120
Copy link
Member

@romandev romandev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is better that previous.
What is our benefit if we move JSTypeTraits to generator?
IMHO, generated code should be smaller if possible.
Because they make us difficult to control them and are poor readbility.

As you know, C++ template is already a kind of code generator. Isn't it better to use it?

@corona10
Copy link
Contributor Author

corona10 commented Oct 9, 2017

@romandev I definitely agree with your opinion.

  1. But in our current implementation, without specialization for specific type. C++ template code generating is not propery works.(If I am wrong please point it!) By generating js_type_traits we can avoid to write every single specialization.

  2. Also I found that specializtiong specific types (e.g int and long) should be differently handling depends on system. On my osx system, int type specializtion not worked but long type worked. But on the travis build system long type specialization not worked.
    I know this could be solved by conditinally compile definitions. But this approach could be one of the solution.

I am writing a proof of concept of this opnion. Until then we can delay this PR to be droped or not :)

  1. In the future, we can reduce unused js_types depending on user's implementations.

@romandev
Copy link
Member

romandev commented Oct 9, 2017

@corona10 Thank you for detailed explanation. I left some inline comments.

@romandev I definitely agree with your opinion.

  1. But in our current implementation, without specialization for specific type. C++ template code generating is not propery works.(If I am wrong please point it!) By generating js_type_traits we can avoid to write every single specialization.

Yeah, I think template specialization was mis-choice. It doesn't cover implicit type converting such as int, long. Because there was no explicit specialization. In other words, after your patch, we still have to define template explicit specialization.

  1. Also I found that specializtiong specific types (e.g int and long) should be differently handling depends on system. On my osx system, int type specializtion not worked but long type worked. But on the travis build system long type specialization not worked.
    I know this could be solved by conditinally compile definitions. But this approach could be one of the solution.

If we remove template specialization and then define method overloading + explicit method call, we can resolve this issue as follows:

// The following function will cover int8_t, int16_t, int32_t including implicit converting.
Napi::Value JSTypeTraitsForInt(int number) {
    ...
}

Napi::Value JSTypeTraits(bool boolean) {
    ...
}

Napi::Value JSTypeTraits(const std::string& string) {
    ...
}

// The following function will cover both of float and double.
Napi::Value JSTypeTraits(double float_point) {
    ...
}

I am writing a proof of concept of this opnion. Until then we can delay this PR to be droped or not :)

  1. In the future, we can reduce unused js_types depending on user's implementations.

It might be but micro-optimization (for basic type) might be trivial for now, IMHO.

@corona10 corona10 closed this Oct 9, 2017
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