Skip to content
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

Documentation: Unclear if Buffer buf[i] is bounds-checked #11244

Closed
joliss opened this issue Feb 8, 2017 · 5 comments · Fixed by #11286
Closed

Documentation: Unclear if Buffer buf[i] is bounds-checked #11244

joliss opened this issue Feb 8, 2017 · 5 comments · Fixed by #11286
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.

Comments

@joliss
Copy link
Contributor

joliss commented Feb 8, 2017

From the buf[i] documentation for Buffer, it's unclear whether the following code produces undefined behavior due to out-of-bounds access or does nothing:

new Buffer('')[1000] = 42

I think this should be documented.

(I assume it's bounds-checked since console.log(new Buffer('')[1000]) prints undefined rather than 0, but I haven't checked the source.)

@mscdex mscdex added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Feb 8, 2017
@seishun seishun added good first issue Issues that are suitable for first-time contributors. and removed good first issue Issues that are suitable for first-time contributors. labels Feb 9, 2017
@seishun
Copy link
Contributor

seishun commented Feb 9, 2017

That part of the documentation is a relic from the past when Buffer didn't inherit from Uint8Array. Now it's technically unnecessary since the behavior of Buffer's array index syntax is exactly as described in ECMAScript 2015 spec. Maybe it should be noted that buf[i] is inherited from Uint8Array in the docs though. /cc @nodejs/collaborators

@ronkorving
Copy link
Contributor

@seishun I would argue though that people shouldn't have to look at 2 sets of documentation (Buffer and Uint8Array) just to figure out what the behavior is. That indirection is not particularly user friendly I think.

@seishun
Copy link
Contributor

seishun commented Feb 10, 2017

@ronkorving I agree that basic information about buf[i] should be present in the Buffer documentation, but we should draw the line somewhere. For example, do we want to provide detailed information about how indexed access on a Buffer always ignores the prototype chain? That would take up a lot of space, and most people don't care about such fine details. And those who do would probably prefer to read the spec rather than its retelling in the Node.js docs.

@ronkorving
Copy link
Contributor

ronkorving commented Feb 10, 2017

I do think that array-like-access should be documented to say whether it will throw or return undefined when out of bounds. But let's get some more input from others on this.

@trevnorris
Copy link
Contributor

The documentation does have the buf[index] section, so would be easy enough to include it there.

MylesBorins pushed a commit that referenced this issue Mar 28, 2017
PR-URL: #11286
Fixes: #11244
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
addaleax pushed a commit that referenced this issue Apr 19, 2017
PR-URL: #11286
Fixes: #11244
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
MylesBorins pushed a commit that referenced this issue Apr 24, 2017
PR-URL: #11286
Fixes: #11244
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
MylesBorins pushed a commit that referenced this issue Apr 24, 2017
PR-URL: #11286
Fixes: #11244
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
PR-URL: nodejs/node#11286
Fixes: nodejs/node#11244
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants