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

[seishun]

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]

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]

(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]

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]

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]

<!-- YAML added: replaceme --> 

:)

[addaleax]

Can you use the overload that takes a context argument?

[seishun]

@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]

is this accurate?

[addaleax]

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

[seishun]

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

[TimothyGu]

@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]

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]

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]

@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]

32 → 64

[tniessen]

32 → 64

[seishun]

@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]

cc @nodejs/collaborators need more input.

[benjamingr]

@littledan how far is BigInt from V8?

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

[mcollina]

IMHO we should wait for BigInt.

[bnoordhuis]

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]

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]

@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]

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]

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]

@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]

Yes, I'm okay with that.

[seishun]

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

[mscdex]

@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]

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

[mscdex]

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.