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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

robinchrist
Copy link

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)

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Mostly LGTM minus two things:

  1. This needs documentation and a regression test.

  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".

I'd be okay with leaving out NAN_ENABLE_STRING_VIEW unless you feel there's a valid reason people would want to disable the overload. I can't think of one myself.

@robinchrist
Copy link
Author

robinchrist commented Dec 1, 2019

@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
Copy link
Member

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.

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