-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: fix -Wunused-result warning in e38bade #6276
Conversation
This patch fixes the warning introduced by the changes in e38bade. Ref: nodejs#6092
aa99825
to
52a40be
Compare
@@ -3229,7 +3229,8 @@ void SetupProcessObject(Environment* env, | |||
|
|||
// pre-set _events object for faster emit checks | |||
Local<Object> events_obj = Object::New(env->isolate()); | |||
events_obj->SetPrototype(env->context(), Null(env->isolate())); | |||
maybe = events_obj->SetPrototype(env->context(), Null(env->isolate())); | |||
CHECK(maybe.FromJust()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could fit on one line as CHECK(events_obj->SetPrototype(env->context(), Null(env->isolate()).FromJust());
, couldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bnoordhuis Nope, it comes to 83 characters
LGTM |
1 similar comment
LGTM |
LGTM |
1 similar comment
LGTM |
Landing this now, as its a trivial change and got 4 CTC LGTMs. Landed in 697790c |
This patch fixes the warning introduced by the changes in e38bade. Ref: nodejs#6092 PR-URL: nodejs#6276 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
Checklist
Affected core subsystem(s)
src
Description of change
This patch fixes the warning introduced by the changes in e38bade.
cc @Fishrock123 @jasnell @mscdex
Ref: #6092