buffer: add {read|write}[U]Int64{BE|LE} methods #15152

seishun · 2017-09-02T20:13:01Z

This is basically a resurrection of @TooTallNate's PR #1750 with a couple differences:

read[U]Int64{BE|LE} always returns strings to make the output more predictable.
No address() method, and thus no changes to the inspect output for Buffers

I'm not sure how to deal with attribution. @TooTallNate did most of the work here, but I also did significant work rebasing and updating.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

buffer

mscdex · 2017-09-02T20:52:04Z

I'm leaning more towards -1 because of the current lack of support in the language (and thus an inconsistency with the return values for the 64-bit read*() functions). Besides, I believe we already cram 64-bit integers into JS numbers in places in core, so returning a string would be deviating from that. Some end users may not even care about a potential loss in precision (e.g. when implementing a protocol that utilizes a 64-bit field whose values never exceed Number.MAX_SAFE_INTEGER), so they would rather have a number value instead of a string.

However, it is probably best to just wait until something like the BigInt ES proposal gets accepted before adding 64-bit integer support.

seishun · 2017-09-02T20:59:17Z

(edited because the message above was edited)

Besides, I believe we already cram 64-bit integers into JS numbers in places in core, so returning a string would be deviating from that.

Do we do that anywhere where the integer could be greater than Number.MAX_SAFE_INTEGER?

Some end users may not even care about a potential loss in precision (e.g. when implementing a protocol that utilizes a 64-bit field whose values never exceed Number.MAX_SAFE_INTEGER), so they would rather have a number value instead of a string.

They can use + to convert it to a number. The other way around wouldn't work.

Would you be less opposed to returning a number when it's less than Number.MAX_SAFE_INTEGER, as the original PR did?

Also, it is probably best to just wait until something like the BigInt ES proposal gets accepted before adding 64-bit integer support.

That's far in the future.

Besides, with BigInt reading 64-bit integers would still be inconsistent.

mscdex · 2017-09-02T21:04:42Z

Besides, I believe we already cram 64-bit integers into a JS number value in places in core.

So?

So there would be an inconsistency in how we deal with 64-bit integer values in core. I don't think that would be a good thing.

Also, it is probably best to just wait until something like the BigInt ES proposal gets accepted before adding 64-bit integer support.

That's far in the future.

Maybe, maybe not. It's already at stage 3. Besides, we've gone this long without 64-bit integer Buffer read() and write() functions, waiting a little longer won't hurt and it's better to implement it in the most appropriate way from the get-go (rather than having to switch from strings/numbers to BigInt later on in a semver-major which will cause unnecessary pain for end users).

seishun · 2017-09-02T21:27:27Z

So there would be an inconsistency in how we deal with 64-bit integer values in core. I don't think that would be a good thing.

Can you give specific examples?

it's better to implement it in the most appropriate way from the get-go

I'm not convinced that using BigInt would be the most appropriate way. A common usecase for 64-bit integers is some kind of an identifier, in which case a primitive is more convenient (and actually more consistent with other read/write functions).

addaleax

I don’t see anything terribly wrong with this, fwiw.

addaleax · 2017-09-02T23:38:17Z

doc/api/buffer.md



:)

addaleax · 2017-09-02T23:40:46Z

src/node_buffer.cc

Can you use the overload that takes a context argument?

@addaleax This is copied from

node/src/node_buffer.cc

Line 724 in 1b7372f

uint32_t offset = args[1]->Uint32Value();

I could fix it in ReadInt64Generic and make it inconsistent with ReadFloatGeneric, or fix it in both places as part of this PR, or make a separate PR to fix it in ReadFloatGeneric.

addaleax · 2017-09-02T23:41:42Z

doc/api/buffer.md

is this accurate?

addaleax · 2017-09-02T23:43:24Z

src/node_buffer.cc

ditto (using the non-deprecated overload taking a context argument)

@addaleax I didn't realize that this overload is deprecated. Is this documented somewhere?

@seishun Yes:

node/deps/v8/include/v8.h

Lines 2297 to 2309 in 94be2b1

V8_WARN_UNUSED_RESULT Maybe<bool> BooleanValue(Local<Context> context) const;

V8_WARN_UNUSED_RESULT Maybe<double> NumberValue(Local<Context> context) const;

V8_WARN_UNUSED_RESULT Maybe<int64_t> IntegerValue(

Local<Context> context) const;

V8_WARN_UNUSED_RESULT Maybe<uint32_t> Uint32Value(

Local<Context> context) const;

V8_WARN_UNUSED_RESULT Maybe<int32_t> Int32Value(Local<Context> context) const;

V8_DEPRECATE_SOON("Use maybe version", bool BooleanValue() const);

V8_DEPRECATE_SOON("Use maybe version", double NumberValue() const);

V8_DEPRECATE_SOON("Use maybe version", int64_t IntegerValue() const);

V8_DEPRECATE_SOON("Use maybe version", uint32_t Uint32Value() const);

V8_DEPRECATE_SOON("Use maybe version", int32_t Int32Value() const);

And also V8 API changes:

Ongoing and Planned Changes

... APIs that are marked as V8_DEPRECATE_SOON will be marked as V8_DEPRECATED in the future. APIs marked as V8_DEPRECATED will usually be removed after one V8 release.

Introduction of MaybeLocal<> and Maybe<> APIs: bug

...

TimothyGu · 2017-09-03T00:21:37Z

I don't see the necessity of waiting until BigInt. In fact when it's introduced we can add a readBig[U]Int64 to match DataView. IMO returning a string isn't ideal, but the fact that the developer can cast it easily to a number by prefixing a + softens the blow for me.

jasnell · 2017-09-05T17:01:49Z

Just alternatively, the return could be an array similar to that used by process.hrtime() ... which should make calculations faster than string parsing / conversion.

seishun · 2017-09-07T18:54:34Z

@jasnell an array is difficult to convert to either a number or a string, and it would be inconsistent with other read functions which returns primitives, so I'm -1 on that.

tniessen

I wonder which possible use cases this might benefit. Sure, people can read 64 bit values as strings, and can then parse (some of them!) as numbers, but there is not even a guarantee that arithmetic operations with those numbers will be accurate, and there is still no fixed-size arithmetic.

If you decide to return numbers as a decimal representation, that should probably be mentioned in the docs.

tniessen · 2017-09-08T09:15:40Z

doc/api/buffer.md

tniessen · 2017-09-08T09:16:02Z

doc/api/buffer.md

seishun · 2017-09-08T09:44:27Z

@tniessen

I wonder which possible use cases this might benefit. Sure, people can read 64 bit values as strings, and can then parse (some of them!) as numbers, but there is not even a guarantee that arithmetic operations with those numbers will be accurate, and there is still no fixed-size arithmetic.

Not everyone requires arithmetic operations on the values they read or write. As I've mentioned before, 64-bit integers are often used as identifiers, for example SteamID.

seishun · 2017-09-10T16:43:13Z

cc @nodejs/collaborators need more input.

benjamingr · 2017-09-11T00:29:22Z

@littledan how far is BigInt from V8?

Edit: found https://bugs.chromium.org/p/v8/issues/detail?id=6791

mcollina · 2017-09-12T14:14:25Z

IMHO we should wait for BigInt.

bnoordhuis · 2017-09-12T14:28:34Z

Sorry, missed that I was asked to review this. I'm also in the "Waiting for Godot^WBigInt" camp; it's at stage 3, implementation work has started, it won't be long (no C programmer pun intended.)

littledan · 2017-09-12T15:54:34Z

I'm working on BigInt specifically in order to enable these use cases. Strings seem like a functioning workaround, but I'm wondering what's motivating adding this to core right now at a time when BigInts are actually under development.

benjamingr · 2017-09-12T17:21:43Z

@littledan

right now at a time when BigInts are actually under development.

Well, there are several reasons Node.js might do this:

There is no guarantee of when this will actually ship.
Since it's Stage 3, while unlikely it is possible that the proposal will be dropped, and Node.js will end up having to maintain its own BigInt module which will perform worse after they will be removed from V8.

That said, I vote "wait for BigInt" too.

tniessen · 2017-09-12T21:44:50Z

Whether BigInt is coming or not, I am not convinced that we should represent decimals as strings just to make them accessible at all (even though I won't block this PR). I think very few users need to represent 64 bit numbers as decimal strings and even fewer need to use a buffer to convert to and from those numbers. With BigInt coming up, we got a promising alternative, which only reinforces my opinion.

ronkorving · 2017-09-13T08:13:27Z

I believe we should be conservative with breaking (in this case, future) API, and we should always aim for consistency and elegance when it comes to naming things. Given that, I'm also voting we wait for BigInt.

benjamingr

Making it explicit

BridgeAR · 2017-09-13T18:36:32Z

@seishun I am sure this was quite some work but I think this will not land and I would rather close it for now. Are you ok with that? I think it is a good basis for a PR as soon as v8 supports BigInt out of the box!

seishun · 2017-09-13T19:14:11Z

Yes, I'm okay with that.

seishun · 2017-09-23T08:36:00Z

@littledan @mscdex @bnoordhuis Any way I can be informed when a V8 version with BigInt support makes it to Node.js?

mscdex · 2017-09-23T08:58:38Z

@seishun Not really (other than reading V8/node changelogs), but for right now you can track the BigInt repo or the ECMAScript finished proposals list to watch for when the BigInt proposal reaches stage 4 (finished). I doubt V8 would add support for BigInt (behind a flag or otherwise) before it reaches stage 4, but I could be wrong about that as I don't know of their plans.

vsemozhetbyt · 2017-09-23T09:21:01Z

@seishun You can also track this issue: https://bugs.chromium.org/p/v8/issues/detail?id=6791

mscdex · 2017-09-23T17:55:06Z

It does look like they do have it behind a flag (--harmony-bigint) as of a few days ago, although it will be awhile before node sees that feature addition.

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Sep 2, 2017

seishun requested review from bnoordhuis, refack and trevnorris September 2, 2017 20:16

addaleax reviewed Sep 2, 2017

View reviewed changes

tniessen reviewed Sep 8, 2017

View reviewed changes

tniessen added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 8, 2017

seishun added 5 commits September 11, 2017 19:19

buffer: add {read|write}[U]Int64{BE|LE} methods

21fd0ff

add replacemes

dd124df

fix return type in docs

681180d

doc fix

286cc23

use non-deprecated api

55e273b

benjamingr requested changes Sep 13, 2017

View reviewed changes

seishun closed this Sep 13, 2017

seishun mentioned this pull request Mar 30, 2018

buffer: add {read|write}Big[U]Int64{BE|LE} methods #19691

Closed

4 tasks

	V8_WARN_UNUSED_RESULT Maybe<bool> BooleanValue(Local<Context> context) const;
	V8_WARN_UNUSED_RESULT Maybe<double> NumberValue(Local<Context> context) const;
	V8_WARN_UNUSED_RESULT Maybe<int64_t> IntegerValue(
	Local<Context> context) const;
	V8_WARN_UNUSED_RESULT Maybe<uint32_t> Uint32Value(
	Local<Context> context) const;
	V8_WARN_UNUSED_RESULT Maybe<int32_t> Int32Value(Local<Context> context) const;

	V8_DEPRECATE_SOON("Use maybe version", bool BooleanValue() const);
	V8_DEPRECATE_SOON("Use maybe version", double NumberValue() const);
	V8_DEPRECATE_SOON("Use maybe version", int64_t IntegerValue() const);
	V8_DEPRECATE_SOON("Use maybe version", uint32_t Uint32Value() const);
	V8_DEPRECATE_SOON("Use maybe version", int32_t Int32Value() const);

Uh oh!

buffer: add {read|write}[U]Int64{BE|LE} methods #15152

buffer: add {read|write}[U]Int64{BE|LE} methods #15152

Uh oh!

Conversation

seishun commented Sep 2, 2017

Checklist

Affected core subsystem(s)

Uh oh!

mscdex commented Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seishun commented Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seishun commented Sep 2, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 2, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 2, 2017

Choose a reason for hiding this comment

Uh oh!

seishun Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 2, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 2, 2017

Choose a reason for hiding this comment

Uh oh!

seishun Sep 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu Sep 11, 2017

Choose a reason for hiding this comment

Ongoing and Planned Changes

Uh oh!

TimothyGu commented Sep 3, 2017

Uh oh!

jasnell commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seishun commented Sep 7, 2017

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

tniessen Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

seishun commented Sep 8, 2017

Uh oh!

seishun commented Sep 10, 2017

Uh oh!

benjamingr commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Sep 12, 2017

Uh oh!

bnoordhuis commented Sep 12, 2017

Uh oh!

littledan commented Sep 12, 2017

Uh oh!

benjamingr commented Sep 12, 2017

Uh oh!

tniessen commented Sep 12, 2017

Uh oh!

mscdex commented Sep 2, 2017 •

edited

Loading

seishun commented Sep 2, 2017 •

edited

Loading

mscdex commented Sep 2, 2017 •

edited

Loading

seishun Sep 11, 2017 •

edited

Loading

seishun Sep 3, 2017 •

edited

Loading

jasnell commented Sep 5, 2017 •

edited

Loading

benjamingr commented Sep 11, 2017 •

edited

Loading

mscdex commented Sep 23, 2017 •

edited

Loading