Skip to content

Conversation

@tniessen
Copy link
Member

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

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);
}

V8 considers gc(true) legacy, and the new signature is much more expressive.
@tniessentniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 19, 2022
@nodejs-github-botnodejs-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-actionsgithub-actionsbot 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.

@tniessentniessen requested a review from mcollinaJune 19, 2022 14:28
@nodejs-github-bot

This comment was marked as outdated.

Copy link
Member

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

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

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

@nodejs-github-bot
Copy link
Collaborator

@tniessentniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 21, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 21, 2022
@nodejs-github-botnodejs-github-bot merged commit f5ce6b1 into nodejs:mainJun 21, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in f5ce6b1

F3n67u pushed a commit to F3n67u/node that referenced this pull request Jun 24, 2022
V8 considers gc(true) legacy, and the new signature is much more expressive. PR-URL: nodejs#43493 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
V8 considers gc(true) legacy, and the new signature is much more expressive. PR-URL: #43493 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
@targostargos mentioned this pull request Jul 12, 2022
targos pushed a commit that referenced this pull request Jul 20, 2022
V8 considers gc(true) legacy, and the new signature is much more expressive. PR-URL: #43493 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
V8 considers gc(true) legacy, and the new signature is much more expressive. PR-URL: #43493 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
@targostargos mentioned this pull request Aug 3, 2022
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
V8 considers gc(true) legacy, and the new signature is much more expressive. PR-URL: nodejs/node#43493 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@tniessen@nodejs-github-bot@mcollina@Trott@benjamingr@devsnek