-
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
Changes from 1 commit
c31be42
275b17b
54c9dad
90af46a
baad1f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1060,6 +1060,8 @@ assert.throws(function() { | |
Buffer.allocUnsafe(0xFFFFFFFFF); | ||
}, RangeError); | ||
|
||
assert(Buffer.alloc.toString().length < 600, 'Buffer.alloc is not inlineable'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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. |
||
|
||
// https://github.com/nodejs/node/issues/9226 | ||
{ | ||
const buf = Buffer.alloc(4, 'YQ==', 'base64'); | ||
|
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 know this was copied, but there is a typo here: s/interpretted/interpreted/