{"action":"created","issue":{"url":"https://api.github.com/repos/mapbox/node-cpp-skel/issues/54","repository_url":"https://api.github.com/repos/mapbox/node-cpp-skel","labels_url":"https://api.github.com/repos/mapbox/node-cpp-skel/issues/54/labels{/name}","comments_url":"https://api.github.com/repos/mapbox/node-cpp-skel/issues/54/comments","events_url":"https://api.github.com/repos/mapbox/node-cpp-skel/issues/54/events","html_url":"https://github.com/mapbox/node-cpp-skel/issues/54","id":247229468,"number":54,"title":"Use Nan::Utf8String over String::Utf8Value","user":{"login":"springmeyer","id":20300,"avatar_url":"https://avatars1.githubusercontent.com/u/20300?v=4","gravatar_id":"","url":"https://api.github.com/users/springmeyer","html_url":"https://github.com/springmeyer","followers_url":"https://api.github.com/users/springmeyer/followers","following_url":"https://api.github.com/users/springmeyer/following{/other_user}","gists_url":"https://api.github.com/users/springmeyer/gists{/gist_id}","starred_url":"https://api.github.com/users/springmeyer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/springmeyer/subscriptions","organizations_url":"https://api.github.com/users/springmeyer/orgs","repos_url":"https://api.github.com/users/springmeyer/repos","events_url":"https://api.github.com/users/springmeyer/events{/privacy}","received_events_url":"https://api.github.com/users/springmeyer/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":7,"created_at":"2017-08-01T23:16:12Z","updated_at":"2017-08-08T21:55:56Z","closed_at":null,"body":"A common way to get data from a v8::String to pass to C++ is:\r\n\r\n```c++\r\nstd::string str(*v8::String::Utf8Value(info[0].As<String>());\r\n```\r\n\r\nWhile concise (by v8/c++ terms) this can be improved. Instead of `v8::String::Utf8Value` we can use `Nan::Utf8String` and we should validate the input before trying to create a `std::string` by:\r\n\r\n  - ensuring the `info[0]` actually is a string\r\n  - ensuring that if it is a string, then the length is > 0 or the string data is null (more on this below)\r\n\r\nThe `Nan::Utf8String` api both validates strings internally and is fast. It has an [optimized conversion routine](https://github.com/nodejs/nan/blob/7291008d755f658343bcb61f456cb0a6744a6235/nan.h#L907-L924) similar [to node core](https://github.com/nodejs/node/blob/98e54b0bd4854bdb3e2949d1b6b20d6777fb7cde/src/util.cc#L36-L49). And it works across all recent node versions.\r\n\r\nSo we should convert all usage of `String::Utf8Value` to `Nan::Utf8String`. Like the `String::Utf8Value` interface, the `Nan::Utf8String` API exposes is the ability to check the length of the string before accessing in the value: this is a very good idea for safety and robustness. We don't want to risk passing a null string around, which might create unpredictable behavior or a crash like in https://github.com/mapbox/node-fontnik/issues/124.\r\n\r\nTo keep 100% test coverage, after adding lines that check the line, we will need to add a test that tries to pass an empty string, and will need to assert reasonable behavior in this case. So, let's move to code like this:\r\n\r\n```cpp\r\n  Nan::Utf8String utf8_value(info[0]);\r\n  int len = utf8_value.length();\r\n  if (len <= 0) {\r\n     return Nan::ThrowTypeError(\"arg must be a non-empty string\");\r\n  }\r\n  /// work with string data here....\r\n  // e.g. convert to a std::string\r\n  std::string string_copy(*utf8_value, len);\r\n```\r\n\r\nAn additional (albeit minor unless the string is massive) performance win of getting the `len` of the string is that we can pass it to the `std::string` constructor. Doing `std::string string_copy(*utf8_value);` would work, but providing the length allows the `std::string` constructor to avoid calculating the length internally and should be faster since it skips an operation. The `std::string` class has a bewildering number of different constructors. This one we are using that accepts a `const char *` as the first arg and an `int` as the second argument is the `(4)` from http://en.cppreference.com/w/cpp/string/basic_string/basic_string.\r\n\r\n/cc @GretaCB "},"comment":{"url":"https://api.github.com/repos/mapbox/node-cpp-skel/issues/comments/321093257","html_url":"https://github.com/mapbox/node-cpp-skel/issues/54#issuecomment-321093257","issue_url":"https://api.github.com/repos/mapbox/node-cpp-skel/issues/54","id":321093257,"user":{"login":"springmeyer","id":20300,"avatar_url":"https://avatars1.githubusercontent.com/u/20300?v=4","gravatar_id":"","url":"https://api.github.com/users/springmeyer","html_url":"https://github.com/springmeyer","followers_url":"https://api.github.com/users/springmeyer/followers","following_url":"https://api.github.com/users/springmeyer/following{/other_user}","gists_url":"https://api.github.com/users/springmeyer/gists{/gist_id}","starred_url":"https://api.github.com/users/springmeyer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/springmeyer/subscriptions","organizations_url":"https://api.github.com/users/springmeyer/orgs","repos_url":"https://api.github.com/users/springmeyer/repos","events_url":"https://api.github.com/users/springmeyer/events{/privacy}","received_events_url":"https://api.github.com/users/springmeyer/received_events","type":"User","site_admin":false},"created_at":"2017-08-08T21:55:56Z","updated_at":"2017-08-08T21:55:56Z","body":"Good to close 👍 As far as last convo about moving to a `node::Buffer` over `std::string`, my feelins that would be a decent lift, so perhaps something to keep in mind, but not critical to ticket."}}
{"actor":{"display_login":"springmeyer"}}