-
Notifications
You must be signed in to change notification settings - Fork 30k
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.
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
buffer: fix fill
with encoding in Buffer.alloc()
#9238
Closed
not-an-aardvark
wants to merge
5
commits into
nodejs:v4.x-staging
from
not-an-aardvark:buf-alloc-encoding
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
c31be42
buffer: fix `fill` with encoding in Buffer.alloc()
not-an-aardvark 275b17b
squash: use Buffer.from
not-an-aardvark 54c9dad
squash: link to GitHub issue in comment
not-an-aardvark 90af46a
squash: ensure Buffer.alloc is inlineable
not-an-aardvark baad1f6
squash: fix comment typo
not-an-aardvark File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this. There was some discussion awhile back about linting for this, but maybe it would be easier or just as easy to do it this way.
I'm wondering though since it's not really a test per se that we might just put all of these kinds of checks in a separate file where we check the lengths of various commonly used functions? /cc @nodejs/collaborators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to do this for all functions, then I think linting would be easier. But I would assume we have a lot of longer functions that aren't worth shortening.
For what it's worth, apparently TurboFan doesn't take function length into account when inlining (source).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it wouldn't be for all functions, just select ones.
That's interesting about TurboFan, but Crankshaft is still used quite a bit.
EDIT:
It looks like that change was reverted shortly after?It was relanded after that heh...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, in that case linting for it might be kind of annoying, because we would have to explicitly state the list of functions somewhere.
Should we move this discussion to a separate issue? In terms of making sure
Buffer.alloc
can be inlined, I think this test works fine. Having a separate file for inlining checks might be a good idea, but it seems like it's out of scope for this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, my main concern was about adding this particular instance now. Hopefully we can get some input from other @nodejs/collaborators and see what they think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed to having this somewhere, but I don't think it should be part of our standard test suite. This seems very coupled to V8, and potentially specific versions of V8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we find out later on that function length is no longer a concern, then we can always remove this test. At the moment, though, while we apparently consider it worthwhile to keep commonly-used functions under 600 characters, we're relying on code review to make sure that happens. If it's worth making changes to the standard library's code to fulfill this requirement, I think it's also worth adding a test to make it easier to notice that the function is too long.
(Also, keep in mind that this fix is specific to v4.x, so it seems unlikely that it will make it into a different version of V8.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case if we want to lint against this, there is a ready ESLint plugin for that https://www.npmjs.com/package/eslint-plugin-v8-func-inline which we could enable for select functions using comment-based rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and yes, in TurboFan this should be fixed now - it has limit only on AST complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RReverser I tried out that ESLint plugin, but it seems to check the length of the entire file, rather than the length of a function itself. So if a file has many small functions, they will all get reported.