Add a return value to Buffer.write* methods that returns the new total offset after the write is complete #3624

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants

catshow commented Jul 3, 2012

No description provided.

Wouldn't returning the number of bytes written be a bit more consistent (Buffer#write() currently does that)?

Also, needs updates to the documentation.

catshow commented Jul 3, 2012

I thought it would be less to do on the application programmers side because

var offset = 0;
var b = new Buffer(50);
offset = b.writeUInt8(5, offset);

is better than, because one could easily forget to put the plus in.

offset += b.writeUInt8(5, offset);

Owner

bnoordhuis commented Jul 3, 2012

Wouldn't returning the number of bytes written be a bit more consistent (Buffer#write() currently does that)?

I somewhat agree, I somewhat disagree. Consistency is important, returning the new offset is convenient.

Can other committers weigh in?

@catshow: I don't remember if I already asked you this. Can you sign the CLA?

catshow commented Jul 3, 2012

I would say in the writing of the string it is kind of a hack with incomplete writes because of the Buffer._charsWritten variable that gets set after the write. It would be cleaner to return an array (tuple) with [currentOffset, charactersWritten]. However I know that would probably render a bunch of code useless.

I have signed the CLA. I will wait to update the documentation until you make a decision to accept the contribution.

catshow commented Jul 3, 2012

As an alternative we could do a similar trick as the exports.INSPECT_MAX_BYTES thing and change the write method to return the array of [currentOffset, charactersWritten], if the exports.WRITE_RETURNS_ARRAY = true, with it defaulting to false, and switch it at the 1.0 timeframe. Then that would be a consistent programming experience.

Owner

bnoordhuis commented Jul 3, 2012

No. Those functions are supposed to be simple and fast. No spooky action at a distance with globals.

Buffer._charsWritten

That one is nominated for removal. (I had wanted to do that before v0.8 but I forgot.)

catshow commented Jul 3, 2012

How will one know how many characters were written?

isaacs commented Jul 5, 2012

@catshow The problem is that the concept of a "character" is somewhat confusingly defined in this modern age of unicode and surrogate pairs.

I think it should return the number of bytes written. That's not just the pattern elsewhere in the Buffer class, but it's the return value from write() as well.

catshow commented Jul 5, 2012

The problem that I was trying to solve with my contribution, was to eliminate all of the offset accounting that is needed for all of the write functions.

It was brought up that the write function of a string returned the number of bytes. The problem with the string write function is that a character does not equal a byte, and that function may not write all of the bytes of that string. Currently there is a hack that returns the number of bytes that it wrote and the number of characters that it wrote into a class property that gets cleared on the next write. @bnoordhuis said that he was going to eliminate the _charactersWritten thing and I suggested that a tuple be returned with that function to return the current offset and charactersWritten instead.

Regardless I really have no desire to become engaged in the politics of open source software. I just thought it would be easier on users of the buffer class to not have to track where the current offset is. If you guys want to use the patch do so, if not close it. If you decide to use it I will update the docs in a follow up patch.

isaacs commented Jul 5, 2012

Well, I think the idea of the patch here is valid - right now those functions return nothing at all :)

However, returning the new offset is a bit of a divergence from the patterns one would expect from a write() method (even if it is in some ways better, it's also different.)

The string thing is not easily solved. What happens if you try to write Ω and it only writes 0xCE but not 0xA9? What is the "chars written" in that case? It's better to be precise and consistent, I think, and always return the number of bytes that were written into the buffer. You can += with that easily enough, and wrap in higher level libraries for doing structs and other fancier binary stuff.

catshow commented Jul 6, 2012

I really see no precedent for the write method returning a number of bytes written except for a unix file descriptor write. The Buffer class is not a socket, and most JS programmers do not normally write to file descriptors. In java's ByteBuffer the put methods return the ByteBuffer back so multiple calls would easily be chained together. http://docs.oracle.com/javase/7/docs/api/java/nio/ByteBuffer.html That is probably a better solution than the developer having to track the offset any way. So a better modification would to have an internal index that tracks the current offset, and then the developer could just call

var b = new Buffer(50);
b.writeUInt8(2).writeInt32BE(12345)....; without offsets.

@isaacs writing networking software in javascript is a divergence from the norm, and it may possibly be better. In my way of thinking better is always better and it is OK to be different.

My solution to the pattern of tracking position was to write a thin wrapper of the buffer https://github.com/tjfontaine/node-buffercursor it wouldn't be difficult to return this to make chaining easier

@bnoordhuis bnoordhuis commented on the diff Aug 28, 2012

benchmark/buffer_with_offset_return.js
@@ -0,0 +1,36 @@
+function testBufferWithReturn(){
+ var buffer = new Buffer(50);
+ var offset = 0;
+
+ console.time('buffer with return x 100K');
+
+ for(var i = 0; i < 100000; i++){
+ offset = 0;
+
@bnoordhuis

bnoordhuis Aug 28, 2012

Owner

Too much whitespace, remove the newlines.

@bnoordhuis bnoordhuis commented on the diff Aug 28, 2012

benchmark/buffer_with_offset_return.js
+
+ offset = buffer.writeInt16BE(-2, offset);
+
+ offset = buffer.writeInt32BE(-3, offset);
+
+ offset = buffer.writeFloatBE(1.234, offset);
+
+ offset = buffer.writeDoubleBE(1.234, offset);
+ }
+
+ console.timeEnd('buffer with return x 100K');
+}
+
+console.time('buffer with return x 1 Million');
+
+for(var x=0; x<10; x++){
@bnoordhuis

bnoordhuis Aug 28, 2012

Owner

Style: spaces between operators.

Owner

bnoordhuis commented Aug 28, 2012

I want to merge this. Any objections?

@catshow the buffer write methods have been rewritten recently. @bnoordhuis seemed cool with merging it, so want to reapply the changes to master, fix the whitespace issue, update the docs with the new return value and squash the commits?

catshow commented Feb 7, 2013

Sure no problem. By the way the dart people are doing the same sort of thing with their ByteArray class where they pass in the offset and return the new offset. http://api.dartlang.org/docs/trunk/latest/dart_scalarlist/ByteArray.html see any of the set* methods.

Can one of the admins verify this patch?

Owner

bnoordhuis commented Oct 10, 2013

I'm afraid this PR no longer applies but the functionality has been implemented in master. Closing.

bnoordhuis closed this Oct 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment