Documentation mismatch for napi_get_value_string_* functions

[RReverser]

napi_get_value_string_* say e.g.:

[out] result: Number of bytes copied into the buffer including the null. terminator.

However, the actual implementation in Node returns the number of bytes excluding the null terminator (so for an empty string, it will return 0 and not 1).

Not sure if it's implementation or documentation issue, or I was just confused by the wording. cc @nodejs/n-api

[jasongin]

It's a doc issue. I think the doc was originally written when there were separate APIs for getting the string length vs string contents, and then it was not updated correctly when the APIs were redesigned.

The string APIs now are intentionally designed so they can be used to just get the string length (not including null terminator) by passing in a null buffer. So even when you do provide a buffer, the returned value, which is the number of characters copied, does not include the null terminator for consistency.

[RReverser]

The string APIs now are intentionally designed so they can be used to just get the string length (not including null terminator) by passing in a null buffer.

Yup, I agree it's more convenient that way, just wanted to know whether it's docs or implementation that got out of sync.

By the way, is there a specific reason this API is not aligned with napi_get_cb_info? In the latter, you can pass initial count and retrieve actual one via the same pointer to size_t, while string functions accept these as separate arguments.

Given what you said, I suspect the reason was exactly that one referred to initial length excluding null, while the other returned total size of written bytes, but now that in/out have the same meaning, would it be reasonable to align these APIs for consistency? (I can raise a separate issue for that)

[jasongin]

The bufsize input parameter is the size of the buffer, which must include space for the null terminator. So I'm not sure it would make sense to combine that with the output parameter.

[RReverser]

Makes sense I guess...

[taveras]

hello! i'm a first-time contributor, and it seems that the issue highlights a change needed in the
following documentation: doc/api/n-api.md

is it okay if I open a PR for this change?

[gabrielschulhof]

@taveras Absolutely :)