src: simplify AliasedBuffer lifetime management #26196
Conversation
Rely on the V8 garbage collector to take care of managing the lifetime of the underlying memory of an `AliasedBuffer`.
nodejs-github-bot
added
the
c++
addaleax
added
the
author ready
| // allocate new native buffer | ||
| NativeT* new_buffer = Calloc<NativeT>(new_capacity); | ||
| NativeT* new_buffer = static_cast<NativeT*>(ab->GetContents().Data()); |
There was a problem hiding this comment.
There was a problem hiding this comment.
We've made a decision about being explicit if the type is not too verbose: https://github.com/nodejs/node/blob/master/CPP_STYLE_GUIDE.md#using-auto
There was a problem hiding this comment.
But in current codebase, it's already used this way
Line 235 in c70e853
There was a problem hiding this comment.
Also used by v8 by default
Line 7 in 9b4bf7d
There was a problem hiding this comment.
@gengjiawen The example in node_platform.cc you’re referring to uses a more complex type – I would agree with using auto for that. Generally, we’re not being super consistent here, and we’ve had debates about this before. ;)
I think auto would be okay here too, because the static_cast makes things a bit more obvious, but I still think NativeT* is simple enough as a type to be kept this way.
(Basically, I agree with what the linked clang docs are saying, I just think NativeT* is short enough for me.)
There was a problem hiding this comment.
There is an option MinTypeNameLength, disscuss it's value to make auto rule more consistent ?
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html#options
There was a problem hiding this comment.
@gengjiawen It’s just my opinion, but my gut feeling would be something like 10 or 12 characters or so, and only to use auto when the type is obvious (e.g. from new Something() or when using a cast).
|
Resume CI again: https://ci.nodejs.org/job/node-test-pull-request/20937/ |
|
Landed in d1011f6 |
Rely on the V8 garbage collector to take care of managing the lifetime of the underlying memory of an `AliasedBuffer`. PR-URL: #26196 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Rely on the V8 garbage collector to take care of managing the lifetime of the underlying memory of an `AliasedBuffer`. PR-URL: #26196 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Rely on the V8 garbage collector to take care of managing the lifetime of the underlying memory of an `AliasedBuffer`. PR-URL: #26196 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Rely on the V8 garbage collector to take care of managing
the lifetime of the underlying memory of an
AliasedBuffer.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes