Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: simplify AliasedBuffer lifetime management #26196

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

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), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Rely on the V8 garbage collector to take care of managing
the lifetime of the underlying memory of an `AliasedBuffer`.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 18, 2019
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 19, 2019
@addaleax
Copy link
Member Author

// allocate new native buffer
NativeT* new_buffer = Calloc<NativeT>(new_capacity);
NativeT* new_buffer = static_cast<NativeT*>(ab->GetContents().Data());
Copy link
Member

@gengjiawen gengjiawen Feb 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in current codebase, it's already used this way

auto platform_data = static_cast<PerIsolatePlatformData*>(handle->data);

Copy link
Member

@gengjiawen gengjiawen Feb 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also used by v8 by default

modernize-use-auto,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

@addaleax
Copy link
Member Author

@addaleax
Copy link
Member Author

@addaleax
Copy link
Member Author

Landed in d1011f6

@addaleax addaleax closed this Feb 21, 2019
@addaleax addaleax deleted the aliased-buffer-lifetime branch February 21, 2019 21:22
addaleax added a commit that referenced this pull request Feb 21, 2019
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>
addaleax added a commit that referenced this pull request Feb 25, 2019
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>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants