src: malloced_unique_ptr & make_malloced_unique

[refack]

Alternative to #23641

malloced_unique_ptr is just a specialization of std:unique_ptr using free for deletion, and make_malloced_unique<T> is it's factory using Malloc<T>.

Ref: #23641
Ref: #23543 (review)
Ref: #23434

CI: https://ci.nodejs.org/job/node-test-pull-request/17822/

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1231/pipeline

[addaleax]

The crypto changes are not improving the code.

There’s value in grouping all information about a buffer in a single struct in terms of readability, maintaining less individual local variables if they would otherwise only make sense as a pair.

[refack]

if they would otherwise only make sense as a pair.

IMHO they are only loosely make sense as a pair. They are not used as a pair, and are used in association only in:
https://github.com/nodejs/node/blob/13b0bbff04545476127b6b7dbc86297b80045670/src/node_crypto.cc#L4243

The cost of pairing them together should be weight against using a non-optimal abstraction to store them together.

[addaleax]

IMHO they are only loosely make sense as a pair

They define the bounds of array together. Semantically, they are a pair.

They are not used as a pair, and are used in association only in:

I’m not sure what you mean by “in association” – they are used together in multiple calls you are modifying here.

As for the Buffer::New() call, it might even make sense to have a utility for MallocedBuffer → JS Buffer conversion. (But … that opens another can of worms and I’d prefer not to do it at this point. Talking about embedding use cases at the collaborator summit, it might make sense for us to provide a custom memory allocation mechanism for things that end up as ArrayBuffers).

[refack]

They define the bounds of array together. Semantically, they are a pair.

Yes, but as I read it they are not used together, so the cost of keeping them together outweighs the benefits.

As for the Buffer::New() call, it might even make sense to have a utility for MallocedBuffer → JS Buffer conversion. (But … that opens another can of worms and I’d prefer not to do it at this point. Talking about embedding use cases at the collaborator summit, it might make sense for us to provide a custom memory allocation mechanism for things that end up as ArrayBuffers).

Yeah it's tricky. But I do agree that if that existed it would make sense to use a Buffer-like structure all along.
ATM as I read it MallocedBuffer is used mostly for it's RAII properties...

[addaleax]

ATM as I read it MallocedBuffer is used mostly for it's RAII properties...

It’s better than malloc()/free() because of that, yes. It’s also a sensible abstraction for buffers.

They define the bounds of array together. Semantically, they are a pair.

Yes, but as I read it they are not used together, so the cost of keeping them together outweighs the benefits.

What cost are you referring to? There’s zero runtime overhead over having two separate variables, but on the other hand, there’s readability overhead in splitting the struct up.

[fhinkel]

I'd rather we document that MallocedBuffer(T* data, size_t size) assumes that data data was allocated with malloc than delete the method.

[refack]

Reverted unrelated changes, PTAL.
CI: https://ci.nodejs.org/job/node-test-pull-request/18013/