Skip to content

url: add return value to ToUnicode/ToAscii stubs #10893

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.

Sign up for GitHub

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

Closed
poiru wants to merge 3 commits into from
Closed

url: add return value to ToUnicode/ToAscii stubs #10893

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6882047
url: add return value to ToUnicode/ToAscii stubs
poiru Jan 19, 2017
74af4eb
url: convert int return types to bool
poiru Jan 19, 2017
a8438ac
url: simplify string copying
poiru Jan 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
url: add return value to ToUnicode/ToAscii stubs 
This fixes compilation errors like:

  node\src\node_url.cc(134) : error C4716: 'node::url::ToUnicode': must
  return a value
  • Loading branch information
@poiru
poiru committed Jan 19, 2017
commit 68820471a2a1f6b90b87120f8800cf2973445246
2 changes: 2 additions & 0 deletions src/node_url.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,13 @@ namespace url {
static int ToUnicode(std::string* input, std::string* output) {
output->reserve(input->length());
*output = input->c_str();
Copy link
Copy Markdown
Member

@addaleax addaleax Jan 19, 2017

Choose a reason for hiding this comment

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

@jasnell Is there any reason these functions can’t just be *output = *input; return 0;?

Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Jan 19, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@addaleax addaleax Jan 19, 2017

Choose a reason for hiding this comment

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

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.

data() is an alias for c_str() since C++11. ;)

Copy link
Copy Markdown
Contributor Author

@poiru poiru Jan 19, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@addaleax addaleax Jan 19, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@poiru poiru Jan 19, 2017

Choose a reason for hiding this comment

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

Yeah, seems safe, I just wasn't sure of the original intent of *output = input->c_str().

return 0;
}

static int ToASCII(std::string* input, std::string* output) {
output->reserve(input->length());
*output = input->c_str();
return 0;
}

static int IsValidUTF8(std::string* input) {
Expand Down