-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
doc: small fixes in the buffer.md #9795
Conversation
@@ -404,14 +404,14 @@ to initialize a `Buffer` to zeroes. | |||
Example: | |||
|
|||
```js | |||
const buf = new Buffer(5); | |||
const buf = new Buffer(10); |
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.
how is this a fix? why is 10
better than 5
?
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.
See #9786. This increases the example size (with the predicted outputs in the comments) so the output could not be confusing if the example is testing via a file.
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.
Scanned #9876, not finding what you mean. And further confused by "testing via a file". There is no file in this example. PR should standalone, can you describe why this is an improvement?
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.
Sorry for my incomprehensibility. Consider this case: a user copies this example, pastes it in the empty file test.js
, saves it and runs via console or IDE to see the output. He will not see the described output, he will see two identical lines with null bytes all the times. In #9786 I've described the cause. This fix increases the size of possible test.js
so the two lines will be different. I'll try to make some GIFs now to explain more clearly what I mean.
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.
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 guess I don't mind the buffer getting a bit bigger, but if you are hoping that means it will always be non-zero.... I don't think that's true. Your own system proves that the initial values can be anything, right? 10
bytes is not special, AFAICT.
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.
Thus I think the docs should make the behaviour clear, not the examples. Examples show only one scenario, the docs say how it will be for all scenarios. And frankly, I don't get the point of this example, it doesn't add anything to my understanding. The docs already said the memory wouldn't be initialized. But that's another issue, I guess.
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.
"Contents may vary" does mean it can be anything, including zeroed, but in my case, it is always all zeroes. I do not mean it is true for all the systems, but my case shows that for some systems it is. If the output is all zeroes one time and is not all zeroes another time, it would be OK. But persistent zeroes is confusing, it can make a novice think that doc is wrong.
In the #9786 I've linked to stats that show my system not always zero buffers 8 bytes or smaller. It has some correlation between script size and buffer zeroing scheme. Maybe I have a unique situation. But it seems we won't get more stats because I can't make my point clear(
So what should I do? Just not change these fragments?
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 don't know. It clearly confused you, and docs should clarify, not confuse. On the other hand, 10 seems as random as 5. I guess leave it, at least anyone wondering why it was changed can find the PR, this conversation, and understand why it changed.
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.
Thank you for the patience.
So can I remake the PR as I've described below?
@@ -523,14 +523,14 @@ initialized*. The contents of the newly created `Buffer` are unknown and | |||
Example: | |||
|
|||
```js | |||
const buf = Buffer.allocUnsafe(5); | |||
const buf = Buffer.allocUnsafe(10); |
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.
ditto
@@ -1264,7 +1264,7 @@ console.log(buf.lastIndexOf('buffer', 4)); | |||
const utf16Buffer = Buffer.from('\u039a\u0391\u03a3\u03a3\u0395', 'ucs2'); | |||
|
|||
// Prints: 6 | |||
console.log(utf16Buffer.lastIndexOf('\u03a3', null, 'ucs2')); | |||
console.log(utf16Buffer.lastIndexOf('\u03a3', buf.length, 'ucs2')); |
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.
null
didn't work? It looks like the example was trying to show how the second argument has a sensible default, and you have just provided the default value explicitly.
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.
With null
it outputs -1
. See https://github.com/nodejs/node/blob/master/lib/buffer.js#L601 — it seems null
is converted to 0
here, so null
is not a replacement for default value or argument omission.
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.
However, I am not so sure about this now. It seems the doc is right, but the lib code is wrong. See https://github.com/nodejs/node/blob/master/lib/buffer.js#L603. Maybe we should fix the lib instead.
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.
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've created an issue concerning this.
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.
Maybe null
could be changed into undefined
for now (undefined
does print 6
here). And if #9801 is fixed, we could restore the null
.
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'd prefer that this be broken into seperate commits, doc: small fixes in the buffer.md
is an uninformative commit message. The bullet list in the commit body that lists unrelated changes is a sign that it should have been multiple commits in a single PR, and the bullet list that just refers to a github issue isn't helpful, the commit message should stand alone, though linking to an issue for further information can be helpful. In this case, the PR will land with the commits amended to point to the PR, so it would be better to mention the issue in the PR, not the commit message.
@sam-github Well, I could try, but I am a bit confused how to do this. If I just add several commits to the branch (with undoing most changes in the first new one), will not the first obsolete commit remain in the history? Sorry, I am not a git pro, I usually use GitHub interface( |
I've tried to understand the git commands for this case. Can I do |
So maybe I'll try it now. If this causes a mess, I'll just close the PR and recreate it with a link to this conversation. |
@sam-github Does it seem OK now? |
@@ -2306,6 +2306,9 @@ added: v3.0.0 | |||
On 32-bit architectures, this value is `(2^30)-1` (~1GB). | |||
On 64-bit architectures, this value is `(2^31)-1` (~2GB). | |||
|
|||
Note that this is a property on the `buffer` module as returned by |
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.
the word "as" is extraneous
|
On some systems, some first bytes of allocated buffer can be zeroed by default, so the example doesn't work well - the buffer looks zeroed even before the fill. Fixes #9786
@sam-github Should I cc somebody for another review? |
I and a number of folks are at node interactive this week, I'd give it a few days before worrying that you've been forgotten. And I think two reviews are usually wanted, @nodejs/documentation, maybe someone else can review? |
@@ -1223,7 +1223,7 @@ added: v6.0.0 | |||
--> | |||
|
|||
* `value` {String | Buffer | Integer} What to search for | |||
* `byteOffset` {Integer} Where to begin searching in `buf` (not inclusive). | |||
* `byteOffset` {Integer} Where to begin searching in `buf`. | |||
**Default:** [`buf.length`] |
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.
Default is actually buf.length - 1
.
Line 594 in e21e129
byteOffset = dir ? 0 : (buffer.length - 1); |
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.
@silverwind Should I preserve the link? Should it be
[`buf.length - 1`][`buf.length`]
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.
Put the - 1
outside the link.
@@ -1264,7 +1264,7 @@ console.log(buf.lastIndexOf('buffer', 4)); | |||
const utf16Buffer = Buffer.from('\u039a\u0391\u03a3\u03a3\u0395', 'ucs2'); | |||
|
|||
// Prints: 6 | |||
console.log(utf16Buffer.lastIndexOf('\u03a3', null, 'ucs2')); | |||
console.log(utf16Buffer.lastIndexOf('\u03a3', undefined, 'ucs2')); |
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.
Both null and undefined are technically not valid arguments. Maybe make a better example?
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.
@silverwind It is a bit complicated here and I hoped it would be fixed fully when #9801 is addressed. Maybe this could be postponed till that big issue fixed?
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.
Yeah fine with me.
@sam-github, @silverwind Does it require more reviews to land? |
Not strictly, but I'd like to give it a day or two before landing in case someone else wants to chime in. |
PR-URL: nodejs#9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
On some systems, some first bytes of allocated buffer can be zeroed by default, so the example doesn't work well - the buffer looks zeroed even before the fill. Fixes: nodejs#9786 PR-URL: nodejs#9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
the number of discrete changes in this PR makes it a bit difficult to backport. Would someone be willing to manually backport |
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
On some systems, some first bytes of allocated buffer can be zeroed by default, so the example doesn't work well - the buffer looks zeroed even before the fill. Fixes: #9786 PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
On some systems, some first bytes of allocated buffer can be zeroed by default, so the example doesn't work well - the buffer looks zeroed even before the fill. Fixes: #9786 PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #9795 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Checklist
Affected core subsystem(s)
doc, buffer
Description of change
var
=>const
/let
.*As to
null
=>undefined
in point 3, it is a temporary fix untill #9801 is addressed.