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

src: move version metadata into node_metadata{.h, .cc} #24774

Closed
wants to merge 3 commits into from
Closed

src: move version metadata into node_metadata{.h, .cc} #24774

wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

src: move READONLY_* macros into util.h

Move these macros to util.h so they can be shared among different
C++ files.
Also, renames READONLY_BOOLEAN_PROPERTY to READONLY_TRUE_PROPERTY
(since it sets the property to true), and use ToV8Value in
READONLY_STRING_PROPERTY.

src: move version metadata into node_metadata{.h, .cc}

This patch moves the computation of version metadata from node.cc
into node_metadata{.h, .cc}, and creates a macro that can be
used to iterate over the available version keys (v8, uv, .etc).
This makes the code clearer as now we no longer need to add
all the headers in node.cc just to compute the versions, and
makes it easier to reuse the version definitions.

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

@joyeecheung
src: move READONLY_* macros into util.h
af8a225 
Move these macros to util.h so they can be shared among different
C++ files.
Also, renames `READONLY_BOOLEAN_PROPERTY` to `READONLY_TRUE_PROPERTY`
(since it sets the property to true), and use `ToV8Value` in
`READONLY_STRING_PROPERTY`.
@joyeecheung
src: move version metadata into node_metadata{.h, .cc}
1df332a 
This patch moves the computation of version metadata from node.cc
into node_metadata{.h, .cc}, and creates a macro that can be
used to iterate over the available version keys (v8, uv, .etc).
This makes the code clearer as now we no longer need to add
all the headers in node.cc just to compute the versions, and
makes it easier to reuse the version definitions.
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1766/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 1, 2018
@joyeecheung
Copy link
Member Author

cc @addaleax @jasnell
CI: https://ci.nodejs.org/job/node-test-pull-request/19123/

richardlau
richardlau approved these changes Dec 1, 2018
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

Nice!

joyeecheung
joyeecheung commented Dec 1, 2018
View reviewed changes
node.gyp Outdated
@@ -370,6 +370,7 @@
'src/node_url.cc',
'src/node_util.cc',
'src/node_v8.cc',
'src/node_metadata.cc',
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, should've sort these

@joyeecheung
Copy link
Member Author

joyeecheung commented Dec 2, 2018 •
edited
Loading

@mmarchini BTW, it should be possible to make these directly available as post-mortem metadata that can be consumed by llnode, same goes for the other recent additions like node_binding.cc and node_native_module.cc, maybe we should design some common interface/macros to do this..

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 3, 2018
@joyeecheung
Copy link
Member Author

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

@joyeecheung
Copy link
Member Author

Resume: https://ci.nodejs.org/job/node-test-pull-request/19167/

@Trott
Copy link
Member

Trott commented Dec 4, 2018

Landed in ff7d053...d17d7bd

@Trott Trott closed this Dec 4, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 4, 2018
@joyeecheung @Trott
src: move READONLY_* macros into util.h
7761881 
Move these macros to util.h so they can be shared among different
C++ files.
Also, renames `READONLY_BOOLEAN_PROPERTY` to `READONLY_TRUE_PROPERTY`
(since it sets the property to true), and use `ToV8Value` in
`READONLY_STRING_PROPERTY`.

PR-URL: nodejs#24774
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 4, 2018
@joyeecheung @Trott
src: move version metadata into node_metadata{.h, .cc}
d17d7bd 
This patch moves the computation of version metadata from node.cc
into node_metadata{.h, .cc}, and creates a macro that can be
used to iterate over the available version keys (v8, uv, .etc).
This makes the code clearer as now we no longer need to add
all the headers in node.cc just to compute the versions, and
makes it easier to reuse the version definitions.

PR-URL: nodejs#24774
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@joyeecheung joyeecheung mentioned this pull request Dec 5, 2018
Closed
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
@joyeecheung @BridgeAR
src: move READONLY_* macros into util.h
53b59b4 
Move these macros to util.h so they can be shared among different
C++ files.
Also, renames `READONLY_BOOLEAN_PROPERTY` to `READONLY_TRUE_PROPERTY`
(since it sets the property to true), and use `ToV8Value` in
`READONLY_STRING_PROPERTY`.

PR-URL: #24774
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
@joyeecheung @BridgeAR
src: move version metadata into node_metadata{.h, .cc}
7c70b61 
This patch moves the computation of version metadata from node.cc
into node_metadata{.h, .cc}, and creates a macro that can be
used to iterate over the available version keys (v8, uv, .etc).
This makes the code clearer as now we no longer need to add
all the headers in node.cc just to compute the versions, and
makes it easier to reuse the version definitions.

PR-URL: #24774
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
Merged
4 tasks
This was referenced Dec 7, 2018
Closed
Closed
Closed
Closed
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@joyeecheung @refack
src: move READONLY_* macros into util.h
f33de1f 
Move these macros to util.h so they can be shared among different
C++ files.
Also, renames `READONLY_BOOLEAN_PROPERTY` to `READONLY_TRUE_PROPERTY`
(since it sets the property to true), and use `ToV8Value` in
`READONLY_STRING_PROPERTY`.

PR-URL: nodejs#24774
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@joyeecheung @refack
src: move version metadata into node_metadata{.h, .cc}
c5bc716 
This patch moves the computation of version metadata from node.cc
into node_metadata{.h, .cc}, and creates a macro that can be
used to iterate over the available version keys (v8, uv, .etc).
This makes the code clearer as now we no longer need to add
all the headers in node.cc just to compute the versions, and
makes it easier to reuse the version definitions.

PR-URL: nodejs#24774
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax addaleax mentioned this pull request May 13, 2019
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell 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. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@joyeecheung @nodejs-github-bot @Trott @jasnell @cjihrig @richardlau @BethGriggs