Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

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

7 participants

@catshow

No description provided.

@TooTallNate

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

@TooTallNate

Also, needs updates to the documentation.

@catshow

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);

@bnoordhuis

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

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

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.

@bnoordhuis

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

How will one know how many characters were written?

@isaacs

@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

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

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

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.

@tjfontaine

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
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;
+

Too much whitespace, remove the newlines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on the diff
benchmark/buffer_with_offset_return.js
((17 lines not shown))
+
+ 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++){

Style: spaces between operators.

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

I want to merge this. Any objections?

@trevnorris
Owner

@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

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.

@Nodejs-Jenkins
Collaborator

Can one of the admins verify this patch?

@bnoordhuis

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

@bnoordhuis bnoordhuis closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 1, 2012
  1. @catshow
Commits on Jul 3, 2012
  1. @catshow
This page is out of date. Refresh to see the latest.
Showing with 58 additions and 12 deletions.
  1. +36 −0 benchmark/buffer_with_offset_return.js
  2. +22 −12 lib/buffer.js
View
36 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;
+

Too much whitespace, remove the newlines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ offset = buffer.writeUInt8(1, offset);
+
+ offset = buffer.writeUInt16BE(2, offset);
+
+ offset = buffer.writeUInt32BE(3, offset);
+
+ offset = buffer.writeInt8(-1, offset);
+
+ 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++){

Style: spaces between operators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ testBufferWithReturn();
+}
+
+console.timeEnd('buffer with return x 1 Million');
View
34 lib/buffer.js
@@ -876,6 +876,7 @@ Buffer.prototype.writeUInt8 = function(value, offset, noAssert) {
}
buffer[offset] = value;
+ return offset + 1;
};
function writeUInt16(buffer, value, offset, isBigEndian, noAssert) {
@@ -902,14 +903,15 @@ function writeUInt16(buffer, value, offset, isBigEndian, noAssert) {
buffer[offset + 1] = (value & 0xff00) >>> 8;
buffer[offset] = value & 0x00ff;
}
+ return offset + 2;
}
Buffer.prototype.writeUInt16LE = function(value, offset, noAssert) {
- writeUInt16(this, value, offset, false, noAssert);
+ return writeUInt16(this, value, offset, false, noAssert);
};
Buffer.prototype.writeUInt16BE = function(value, offset, noAssert) {
- writeUInt16(this, value, offset, true, noAssert);
+ return writeUInt16(this, value, offset, true, noAssert);
};
function writeUInt32(buffer, value, offset, isBigEndian, noAssert) {
@@ -940,14 +942,15 @@ function writeUInt32(buffer, value, offset, isBigEndian, noAssert) {
buffer[offset + 1] = (value >>> 8) & 0xff;
buffer[offset] = value & 0xff;
}
+ return offset + 4;
}
Buffer.prototype.writeUInt32LE = function(value, offset, noAssert) {
- writeUInt32(this, value, offset, false, noAssert);
+ return writeUInt32(this, value, offset, false, noAssert);
};
Buffer.prototype.writeUInt32BE = function(value, offset, noAssert) {
- writeUInt32(this, value, offset, true, noAssert);
+ return writeUInt32(this, value, offset, true, noAssert);
};
@@ -1032,6 +1035,7 @@ Buffer.prototype.writeInt8 = function(value, offset, noAssert) {
} else {
buffer.writeUInt8(0xff + value + 1, offset, noAssert);
}
+ return offset + 1;
};
function writeInt16(buffer, value, offset, isBigEndian, noAssert) {
@@ -1056,14 +1060,15 @@ function writeInt16(buffer, value, offset, isBigEndian, noAssert) {
} else {
writeUInt16(buffer, 0xffff + value + 1, offset, isBigEndian, noAssert);
}
+ return offset + 2;
}
Buffer.prototype.writeInt16LE = function(value, offset, noAssert) {
- writeInt16(this, value, offset, false, noAssert);
+ return writeInt16(this, value, offset, false, noAssert);
};
Buffer.prototype.writeInt16BE = function(value, offset, noAssert) {
- writeInt16(this, value, offset, true, noAssert);
+ return writeInt16(this, value, offset, true, noAssert);
};
function writeInt32(buffer, value, offset, isBigEndian, noAssert) {
@@ -1088,14 +1093,15 @@ function writeInt32(buffer, value, offset, isBigEndian, noAssert) {
} else {
writeUInt32(buffer, 0xffffffff + value + 1, offset, isBigEndian, noAssert);
}
+ return offset + 4;
}
Buffer.prototype.writeInt32LE = function(value, offset, noAssert) {
- writeInt32(this, value, offset, false, noAssert);
+ return writeInt32(this, value, offset, false, noAssert);
};
Buffer.prototype.writeInt32BE = function(value, offset, noAssert) {
- writeInt32(this, value, offset, true, noAssert);
+ return writeInt32(this, value, offset, true, noAssert);
};
function writeFloat(buffer, value, offset, isBigEndian, noAssert) {
@@ -1117,14 +1123,16 @@ function writeFloat(buffer, value, offset, isBigEndian, noAssert) {
require('buffer_ieee754').writeIEEE754(buffer, value, offset, isBigEndian,
23, 4);
+
+ return offset + 4;
}
Buffer.prototype.writeFloatLE = function(value, offset, noAssert) {
- writeFloat(this, value, offset, false, noAssert);
+ return writeFloat(this, value, offset, false, noAssert);
};
Buffer.prototype.writeFloatBE = function(value, offset, noAssert) {
- writeFloat(this, value, offset, true, noAssert);
+ return writeFloat(this, value, offset, true, noAssert);
};
function writeDouble(buffer, value, offset, isBigEndian, noAssert) {
@@ -1146,12 +1154,14 @@ function writeDouble(buffer, value, offset, isBigEndian, noAssert) {
require('buffer_ieee754').writeIEEE754(buffer, value, offset, isBigEndian,
52, 8);
+
+ return offset + 8;
}
Buffer.prototype.writeDoubleLE = function(value, offset, noAssert) {
- writeDouble(this, value, offset, false, noAssert);
+ return writeDouble(this, value, offset, false, noAssert);
};
Buffer.prototype.writeDoubleBE = function(value, offset, noAssert) {
- writeDouble(this, value, offset, true, noAssert);
+ return writeDouble(this, value, offset, true, noAssert);
};
Something went wrong with that request. Please try again.