url: add return value to ToUnicode/ToAscii stubs#10893
url: add return value to ToUnicode/ToAscii stubs#10893poiru wants to merge 3 commits intonodejs:masterfrom poiru:url-fix-compilation
Conversation
This fixes compilation errors like: node\src\node_url.cc(134) : error C4716: 'node::url::ToUnicode': must return a value
nodejs-github-bot
added
c++
src/node_url.cc
Outdated
| @@ -131,11 +131,13 @@ namespace url { | |||
| static int ToUnicode(std::string* input, std::string* output) { | |||
| output->reserve(input->length()); | |||
| *output = input->c_str(); | |||
There was a problem hiding this comment.
@jasnell Is there any reason these functions can’t just be *output = *input; return 0;?
There was a problem hiding this comment.
On that subject: as a slightly more wide-ranging cleanup, how about changing the return value from int to bool? It's already effectively boolean in that it returns either 0 or -1.
Also: it looks like both the ICU and non-ICU implementations should be able to use input->data() instead of input->c_str(). Avoids an unnecessary reallocation.
There was a problem hiding this comment.
Also: it looks like both the ICU and non-ICU implementations should be able to use
input->data()instead ofinput->c_str(). Avoids an unnecessary reallocation.
data() is an alias for c_str() since C++11. ;)
There was a problem hiding this comment.
I actually was going to do this, but *output = *input is not equivalent to *output = input->c_str() if the string contains null bytes so I decided to leave it alone. Seems like that is not a concern so I'll go ahead and change it.
There was a problem hiding this comment.
Good point – if I’m reading the code correctly, this should be *output = *input? At least for ToASCII, there’s an explicit check for null bytes just below the call to it.
There was a problem hiding this comment.
Yeah, seems safe, I just wasn't sure of the original intent of *output = input->c_str().
jasnell
left a comment
jasnell
left a comment
There was a problem hiding this comment.
LGTM with the edits suggested by @bnoordhuis and @addaleax
|
@jasnell Review feedback addressed, please retrigger CI. Thanks! |
This fixes compilation errors like: node\src\node_url.cc(134) : error C4716: 'node::url::ToUnicode': must return a value PR-URL: #10893 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
|
Landed in dcab88d |
This fixes compilation errors like: node\src\node_url.cc(134) : error C4716: 'node::url::ToUnicode': must return a value PR-URL: nodejs#10893 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
This fixes compilation errors like: node\src\node_url.cc(134) : error C4716: 'node::url::ToUnicode': must return a value PR-URL: nodejs#10893 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
This fixes compilation errors like: node\src\node_url.cc(134) : error C4716: 'node::url::ToUnicode': must return a value PR-URL: nodejs#10893 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
This fixes compilation errors like: node\src\node_url.cc(134) : error C4716: 'node::url::ToUnicode': must return a value PR-URL: nodejs#10893 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
7 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
url