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: add READONLY_STRING_PROPERTY and simplify config #22222

Closed
wants to merge 2 commits into from

Conversation

maclover7
Copy link
Contributor

@maclover7 maclover7 commented Aug 9, 2018

Bit of tidying up where we set different config values.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Bit of tidying up where we set different config values..
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 9, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM but can we fast track this? I like it, but it’s, like, very directly conflicting with what I’m working on at this point. 😄

@@ -29,6 +29,18 @@ using v8::Value;
True(isolate), ReadOnly).FromJust(); \
} while (0)

#define READONLY_STRING_PROPERTY(obj, str, val) \
do { \
obj->DefineOwnProperty(context, \
Copy link
Member

Choose a reason for hiding this comment

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

It’s a best practice to parenthesize macro parameters, i.e. use (obj)->DefineOwnProperty(…), even if this is probably not an issue for now :)

String::NewFromUtf8( \
isolate, \
val.data(), \
v8::NewStringType::kNormal).ToLocalChecked(), \
Copy link
Member

Choose a reason for hiding this comment

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

tiny nit: 4 spaces for statement continuations

@maclover7
Copy link
Contributor Author

@maclover7
Copy link
Contributor Author

@maclover7 maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 11, 2018
@maclover7
Copy link
Contributor Author

Landed in 30b5b84

@maclover7 maclover7 closed this Aug 12, 2018
@maclover7 maclover7 deleted the jm-node-config branch August 12, 2018 00:28
maclover7 added a commit that referenced this pull request Aug 12, 2018
Bit of tidying up where we set different config values.

PR-URL: #22222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Aug 12, 2018
Bit of tidying up where we set different config values.

PR-URL: #22222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
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.

7 participants