Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Rewrite constexpr with template metaprogram #127
Conversation
|
I have not tested it on VS2013. I do not have it installed, but I do not see why it would not work there. |
|
Ok, let's not worry about it then. IIRC node dropped support for building extensions using VS2013 anyway. |
kkoopa
changed the title
|
CI run on master: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api/ hmm seems to fail I think it needs to be rebased to be able to validate. @kfarnung do you think this is still something we should pull in (it has been a long time). |
|
I guess my question is what benefit we get from switching away from constexpr. I'm not a compiler expert, but presumably there's some compatibility or other benefit from this change. @kkoopa can you elaborate here? I'm not opposed to this change assuming we can get the code rebased and passing the CI. |
|
IIRC, it was for some compatibility reason. I think it pertained to older versions of Visual Studio not fully supporting all constexpr constructs.
…On September 28, 2018 8:46:39 PM GMT+03:00, Kyle Farnung ***@***.***> wrote:
I guess my question is what benefit we get from switching away from
constexpr. I'm not a compiler expert, but presumably there's some
compatibility or other benefit from this change. @kkoopa can you
elaborate here?
I'm not opposed to this change assuming we can get the code rebased and
passing the CI.
|
I think it's a win-win situation:
Old implementation will always compile and evaluate at runtime:
thumbs up for resolving merge conflicts |
|
I looked at the code and can clarify my previous statement: It was for getting rid of the check for |
|
@kkoopa I think you only have to resolve the conflicts so that it's reviewed again |
| static const napi_typedarray_type value = napi_##TYPE##_array; \ | ||
| } | ||
|
|
||
| X(int8); |
There was a problem hiding this comment.
For me X macro is doing to much concat magic. From L1565 you have to impl the types not matching the X macro. Wouldn't it be easier to write more, but without magic?
#define XX(TYPE, NAPI_TYPEDARRAY_TYPE) \
template <> struct TypedArray::type_translator<TYPE> { \
static const napi_typedarray_type value = NAPI_TYPEDARRAY_TYPE; \
}
XX(int8_t, napi_int8_array);
// ...
XX(float, napi_float32_array);There was a problem hiding this comment.
Yes, that would be better since there are several special cases now.
Get rid of constexpr