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

Possible Buffer.concat documentation vagueness #3219

Closed
Martii opened this issue Oct 6, 2015 · 13 comments
Closed

Possible Buffer.concat documentation vagueness #3219

Martii opened this issue Oct 6, 2015 · 13 comments
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.

Comments

@Martii
Copy link

Martii commented Oct 6, 2015

At the current documentation for Buffer.concat the second parameter explanation seems a little vague.

On our project we're trying to avoid a possible use case issue and it would be helpful to definitively explain if totalLength is Byte length or encoded length e.g. String length, or other... otherwise we may have an issue down the road pop-up.

It would be super helpful to clarify this in the documentation. :)

TIA

@trevnorris trevnorris added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Oct 6, 2015
@trevnorris
Copy link
Contributor

@Martii Are you passing strings to Buffer#concat()?

@Martii
Copy link
Author

Martii commented Oct 6, 2015

@trevnorris
No... it is an actual list of Buffers (Array).

@Martii
Copy link
Author

Martii commented Oct 6, 2015

Here's a console dump of a sample object:

[ <Buffer 28 66 75 6e 63 74 69 6f 6e 20 28 29 20 7b 0a 20 20 22 75 73 65 20 73 74 72 69 63 74 22 3b 0a 0a 2f 2f 20 3d 3d 55 73 65 72 53 63 72 69 70 74 3d 3d 0a ... >,
  <Buffer 65 20 63 6f 6d 6d 61 73 20 74 6f 20 73 65 70 61 72 61 74 65 20 6b 65 79 73 3c 2f 65 6d 3e 27 2c 0a 20 20 20 20 20 20 20 20 20 20 22 64 65 66 61 75 6c ... > ]

@trevnorris
Copy link
Contributor

totalLength is a sum of all buffer.length in the array. It doesn't care if any of the buffers contain strings.

@Martii
Copy link
Author

Martii commented Oct 6, 2015

totalLength is a sum of all buffer.length in the array.

So we probably have an issue then. The change in semantics would be useful to alleviate future confusion in the documentation instead of:

totalLength {Number} Total length of the buffers when concatenated

... this is where my confusion is.

I'll run some more detailed site tests with 4 Byte Unicode characters until the docs are updated just to be overly-cautious. Thanks for your assistance. :)

@trevnorris
Copy link
Contributor

@Martii Think I'm missing something. The totalLength can be calculated by the following:

var b1 = new Buffer(10);
var b2 = new Buffer(16);
var b3 = new Buffer(18);

var buffers = [b1, b2, b3];

var totalLength = 0;
for (var i = 0; i < buffers.length; i++) {
  totalLength = buffers[i].length;
}

The for loop is all that happens internally if totalLength isn't passed. I'm not sure what 4 byte unicode characters would have to do with this.

@Martii
Copy link
Author

Martii commented Oct 6, 2015

Your example is a little simpler than what I'm creating for a test User Script...

... but that example would be be perfect in the documentation to demonstrate what totalLength is without using words.

The words used in the current document use "length" a lot and that particular nomenclature can be confusing compared to String.prototype.length sometimes... it's even mentioned in the doc here to clarify for that particular method.

I'm also debug checking the stable release of this API in node@0.12.7 as well as node@latest ... like I said I'm overly cautious sometimes... especially in our project in this area. :)

@trevnorris
Copy link
Contributor

Not a bad thing to be cautious. And a doc PR would be more than welcome. :)

Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Oct 6, 2015
* Thanks to *node* maintainer for the clarification at nodejs/node#3219 of Buffer length versus character length
* Retested on a User Script that contained a lot of Unicode characters in a comment at the beginning then the metadata blocks at the EOF... String.length fails to upload... Buffer length passes on upload with two "chunks"

Applies to OpenUserJS#678
@Martii
Copy link
Author

Martii commented Oct 6, 2015

a doc PR would be more than welcome.

Not sure I could cover all use cases since I'm a little new to this particular global object but I can give it a whirl soon. Thanks again for your help. :)

@Martii
Copy link
Author

Martii commented Oct 8, 2015

Submitted... hopefully correctly. Had to do a little "light" reading and match the current doc style of this .md. ;)

totalLength = buffers[i].length;

Sidenote: missing + added to the example. :)

@seishun
Copy link
Contributor

seishun commented Oct 15, 2015

TBH I don't see any reason to think that a function that takes a list of buffers and returns a buffer would have anything to do with strings. Granted, we all experience brain-farts sometimes, but I don't think the documentation can or should guard against all such cases.

jasnell pushed a commit that referenced this issue Oct 26, 2015
* Add a simple example for buffer.concat
* Change grammar slightly.

Fixes: #3219
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: #3255
rvagg pushed a commit that referenced this issue Oct 26, 2015
* Add a simple example for buffer.concat
* Change grammar slightly.

Fixes: #3219
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: #3255
jasnell pushed a commit that referenced this issue Oct 29, 2015
* Add a simple example for buffer.concat
* Change grammar slightly.

Fixes: #3219
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: #3255
@moander
Copy link
Contributor

moander commented Dec 23, 2015

Maybe add a note about when totalLength is less than the total length of the buffers as well?

@Martii
Copy link
Author

Martii commented Dec 23, 2015

@moander
You will need to probably open a new issue as this one is closed... sounds reasonable. PR's are accepted... since I'm new'ish to this area confirmation will come from someone else but I can give a beginners view point. :)

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

No branches or pull requests

4 participants