Node-addon-api: Allow use of std::string_view with C++17?

[rolftimmermans]

I would love for methods that now take a const std::string& to be able to accept std::string_view

For example:

void ObjectReference::Set(std::string_view utf8name, std::string_view utf8value);

This should be easy enough to implement because all N-API functions that deal with strings can take a pointer & size. There can be a simple #if guard around the method definitions to make sure C++14 and below are still fully supported.

The reason this is useful is when C++17 codebases pass std::string_value around internally, it's wasteful to have to have to allocate a new std::string first. Because there is no guarantee std::string_value refers to a null-terminated string, using const char* would also require making a copy first.

Now, it's of course possible to create new method overloads for each single one that currently takes a const std::string&, but that is quite a few locations. Also, going from 2 to 3 string-like types would mean an explosion in number of method definitions that take two string-like arguments.

Does anyone have a particular opinion about this issue before I start to work on a PR for it?

[NickNaso]

Hi @rolftimmermans,
I think that like you exposed the usage of std::string_view will reduce the memory allocation and will guarantees good performance on some operations over the string.
Do you want to check the __cplusplus value to understand which version of C++ is active?
There are several methods that should add so I will talk of this your idea at next N-API meeting tomorrow and should be great if you could be with us. In case I or other of the team will update you commenting on this issue.

[rolftimmermans]

Yep I think the recommended check would be #if __cplusplus >= 201703L.

What time & via which medium is the meeting? I'll see if I'm able to join!

[NickNaso]

We usually have the meeting at 8 AM Pacific Time see Node.js Calendar
For our meeting we use zoom here the link: https://zoom.us/j/363665824
Hope to see you.

[rolftimmermans]

I won't be able to make it to the meeting, but I'm eager to hear what your opinion is!

[mhdawson]

A few points

• We'd want to have accepting C++ 17 to be op in. Guarded with something like #define NAPI_ALLOW_C17 so that people make the choice to enable usage as opposed to accidentally using features that may not compile when installed on consumers with an earlier compiler.

• We discussed replacing the std:: string entries with something like STRING_PARAM and then have

#define STRING_PARAM to be std::string_view if NAPI_ALLOW_C17 was defined, or const std::string& otherwise.

It seems like the impact to the code/clarity would be relatively limited if this works.

@rolftimmermans Does what we suggested make sense?

[gabrielschulhof]

Actually, the condition for using C++17 features while keeping such use opt-in ought to be

#if (defined(NAPI_ALLOW_C17) && (__cplusplus >= 201703L))

[gabrielschulhof]

... and maybe the name of the opt-in preprocessor directive ought to be NAPI_ALLOW_CPP17 instead of NAPI_ALLOW_C17?

[rolftimmermans]

Makes sense! I'll try to work on a PR.

[github-actions[bot]]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.