-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: refactor reading of options in contextify #8850
Conversation
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.
LGTM
bool display_errors = GetDisplayErrorsArg(env, args, 1); | ||
MaybeLocal<Uint8Array> cached_data_buf = GetCachedData(env, args, 1); | ||
bool produce_cached_data = GetProduceCachedData(env, args, 1); | ||
|
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.
Linter is unhappy about whitespaces here.
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.
Thanks. Fixed.
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.
LGTM with a green CI/linter run
c1ac941
to
958dcdd
Compare
Refactor various functions that read values from the contextify options object. Rather than passing args and the index, pass the value at that index. We use env->isolate() rather than args.GetIsolate(), but since env was constructed from args, this is the same isolate.
958dcdd
to
242e4a2
Compare
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.
LGTM
Refactor various functions that read values from the contextify options object. Rather than passing args and the index, pass the value at that index. We use env->isolate() rather than args.GetIsolate(), but since env was constructed from args, this is the same isolate. PR-URL: nodejs#8850 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in c32cfcb. |
Refactor various functions that read values from the contextify options object. Rather than passing args and the index, pass the value at that index. We use env->isolate() rather than args.GetIsolate(), but since env was constructed from args, this is the same isolate. PR-URL: #8850 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
@fhinkel I have set this as do not land. If you feel it should be backported please feel free to do so |
No need to backport it. Thanks! |
Refactor various functions that read values from the contextify options object. Rather than passing args and the index, pass the value at that index. We use env->isolate() rather than args.GetIsolate(), but since env was constructed from args, this is the same isolate. PR-URL: #8850 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Checklist
make -j8 test
(UNIX) passesAffected core subsystem(s)
src, contextify
Description of change
Refactor various functions that read values from the contextify options object.
Rather than passing
args
and the index, pass the value at that index.We use
env->isolate()
rather thanargs.GetIsolate()
, but sinceenv
was constructed from
args
, this is the same isolate.