search

nodejs / nan

Native Abstractions for Node.js
MIT License
3.29k stars 505 forks source link

Add variant of Nan::New<v8::String> which takes a C++17 string_view #880

Open robinchrist opened 4 years ago

robinchrist commented 4 years ago

This PR adds Nan::New<v8::String>(std::string_view), which makes it easier to handle compile time known strings.

In order to maintan backwards compatibility to C++11, a NAN_HAS_CPLUSPLUS_17 macro is added (which will also be needed for my next PRs), the code is wrapped into NAN_ENABLE_STRING_VIEW guards. If the user wishes to disable the usage of string_view, it can be disabled via adding a define -DNAN_ENABLE_STRING_VIEW=false, otherwise it is automatically enabled if it's supported by the C++ runtime (C++17 and up)

robinchrist commented 4 years ago

@bnoordhuis

2. -DNAN_ENABLE_STRING_VIEW=false won't undefine the macro. defined(NAN_ENABLE_STRING_VIEW) is true because the macro is non-empty, it evaluates to the string "false".

Hmm, I'm not sure whether I can follow. I have prepared a minimum repro here: https://godbolt.org/z/DET2bw

Without any additional NAN_ENABLE_STRING_VIEW flags: Using -std=c++11 -> Output without string_view! Using -std=c++17 -> Output using string_view!

Using -std=c++17 -DNAN_ENABLE_STRING_VIEW=false -> Output without string_view! Using -std=c++17 -DNAN_ENABLE_STRING_VIEW=true -> Output using string_view! Using -std=c++11 -DNAN_ENABLE_STRING_VIEW=true -> Compile error, as expected

Or should I change the meaning of NAN_ENABLE_STRING_VIEW? Basically, instead of "By setting NAN_ENABLE_STRING_VIEW you force the inclusion of string_view" we say "By setting NAN_ENABLE_STRING_VIEW, you can enable or disable string_view, but if it's not compatible, it's disabled anyways"?

EDIT: Just to clarify things: I'm currently not relying on the macro being undefined to disable string_view, instead I decided to use #if. I can switch to ifdefs, if you wish.

EDIT 2: The reason why I've added the NAN_ENABLE_STRING_VIEW flag is that we build our node addon for multiple platforms, and even though some of them claim to have full C++17 support, they do not. Therefore, we need a possibility to disable string_view

bnoordhuis commented 4 years ago

Ah, I think I know where the misunderstanding comes from: -DNAN_ENABLE_STRING_VIEW=false is syntax that's accepted by npm and node-gyp but a) it does something different, and b) it's not what you meant. Okay, objection withdrawn.