Minor fixups #1148
Merged
Minor fixups #1148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| e = Exception::Error(cons2); | ||
| Local<String> path_string; | ||
| if (path != nullptr) { | ||
| // FIXME(bnoordhuis) It's questionable to interpret the file path as UTF-8. |
There was a problem hiding this comment.
What alternative is there?
There was a problem hiding this comment.
One-byte strings or if you want more radical, buffers or typed arrays. It also depends on the platform: file paths are untyped binary blobs on most Unices, but on OS X (i.e. HFS+) and Windows, they're Unicode.
I'm not sure yet what the right answer is, just that our current approach is wrong. :-)
There was a problem hiding this comment.
Good point about different data formats. I was only thinking about string encoding.
|
One non-blocking question. LGTM |
Rewrite ErrnoException() so that it doesn't turn the file path into a string twice. PR-URL: nodejs#1148 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
With --max_old_space_size=12345 it's possible to create a JS heap that is larger than what fits in an unsigned int so use Number::New() rather than Integer::NewFromUnsigned(). Performance-wise, it doesn't matter much. If V8 can fit the double in a SMI, it will. PR-URL: nodejs#1148 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
bnoordhuis
force-pushed
the
minor-fixups
branch
from
12f2fc9
to
2551c1d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
R=@trevnorris?