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.

Sign up for GitHub

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

Jump to bottom

test: replace gc(true) with gc({ type: 'minor' }) #43493

Merged
merged 1 commit into from
Jun 21, 2022
Merged

test: replace gc(true) with gc({ type: 'minor' }) #43493

merged 1 commit into from
Jun 21, 2022

Conversation

tniessen
Copy link
Member

V8 considers gc(true) legacy, and the new signature is much more expressive.

node/deps/v8/src/extensions/gc-extension.cc

Lines 42 to 79 in 94020ac

Maybe<GCOptions> Parse(v8::Isolate* isolate,
const v8::FunctionCallbackInfo<v8::Value>& args) {
// Default values.
auto options =
GCOptions{v8::Isolate::GarbageCollectionType::kFullGarbageCollection,
ExecutionType::kSync};
bool found_options_object = false;
if (args.Length() > 0 && args[0]->IsObject()) {
v8::HandleScope scope(isolate);
auto ctx = isolate->GetCurrentContext();
auto param = v8::Local<v8::Object>::Cast(args[0]);
auto maybe_type = IsProperty(isolate, ctx, param, "type", "minor");
if (maybe_type.IsNothing()) return Nothing<GCOptions>();
if (maybe_type.ToChecked()) {
found_options_object = true;
options.type =
v8::Isolate::GarbageCollectionType::kMinorGarbageCollection;
}
auto maybe_execution =
IsProperty(isolate, ctx, param, "execution", "async");
if (maybe_execution.IsNothing()) return Nothing<GCOptions>();
if (maybe_execution.ToChecked()) {
found_options_object = true;
options.execution = ExecutionType::kAsync;
}
}
// If no options object is present default to legacy behavior.
if (!found_options_object) {
options.type =
args[0]->BooleanValue(isolate)
? v8::Isolate::GarbageCollectionType::kMinorGarbageCollection
: v8::Isolate::GarbageCollectionType::kFullGarbageCollection;
}
return Just<GCOptions>(options);
}

@tniessen
test: replace gc(true) with gc({ type: 'minor' })
38b1b81 
V8 considers gc(true) legacy, and the new signature is much more
expressive.
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 19, 2022
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jun 19, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 19, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen tniessen requested a review from mcollina June 19, 2022 14:28
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
mcollina
mcollina approved these changes Jun 19, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Jun 20, 2022
Open
36 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 20, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Jun 21, 2022
Open
33 tasks
@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 21, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 21, 2022
@nodejs-github-bot nodejs-github-bot merged commit f5ce6b1 into nodejs:main Jun 21, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in f5ce6b1

This was referenced Jun 22, 2022
Open
Open
Open
F3n67u pushed a commit to F3n67u/node that referenced this pull request Jun 24, 2022
@tniessen @F3n67u
test: replace gc(true) with gc({ type: 'minor' })
21b8ed8 
V8 considers gc(true) legacy, and the new signature is much more
expressive.

PR-URL: nodejs#43493
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@github-actions github-actions bot mentioned this pull request Jun 25, 2022
Open
32 tasks
targos pushed a commit that referenced this pull request Jul 12, 2022
@tniessen @targos
test: replace gc(true) with gc({ type: 'minor' })
70d297c 
V8 considers gc(true) legacy, and the new signature is much more
expressive.

PR-URL: #43493
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request Jul 12, 2022
Merged
targos pushed a commit that referenced this pull request Jul 20, 2022
@tniessen @targos
test: replace gc(true) with gc({ type: 'minor' })
007516e 
V8 considers gc(true) legacy, and the new signature is much more
expressive.

PR-URL: #43493
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@tniessen @targos
test: replace gc(true) with gc({ type: 'minor' })
a5df207 
V8 considers gc(true) legacy, and the new signature is much more
expressive.

PR-URL: #43493
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@tniessen @guangwong
test: replace gc(true) with gc({ type: 'minor' })
af79bfa 
V8 considers gc(true) legacy, and the new signature is much more
expressive.

PR-URL: nodejs/node#43493
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@Trott Trott Trott approved these changes

@benjamingr benjamingr benjamingr approved these changes

@devsnek devsnek devsnek approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tniessen @nodejs-github-bot @mcollina @Trott @benjamingr @devsnek