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: add public API to get default node platform #17946

Closed
wants to merge 1 commit into from
Closed

src: add public API to get default node platform #17946

wants to merge 1 commit into from

Conversation

levimm
Copy link

@levimm levimm commented Jan 2, 2018 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
embedding

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 2, 2018
@levimm
Copy link
Author

levimm commented Jan 2, 2018

Reference issue #17859

bnoordhuis
bnoordhuis reviewed Jan 2, 2018
View reviewed changes
src/node.h Outdated
@@ -254,6 +254,8 @@ NODE_EXTERN MultiIsolatePlatform* CreatePlatform(
v8::TracingController* tracing_controller);
NODE_EXTERN void FreePlatform(MultiIsolatePlatform* platform);

NODE_EXTERN MultiIsolatePlatform* GetDefaultPlatform();
Copy link
Member

Choose a reason for hiding this comment

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

Poor name since it implies there is also a non-default platform. Just GetPlatform()?

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking this platform is created in node::Start so it seems to be the default one.
And since we also have CreatePlatform api, I don't want people to think they can get newly created platform with this.

Copy link
Member

Choose a reason for hiding this comment

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

How about GetCurrentPlatform()?

src/node.cc Outdated
@@ -1,4 +1,4 @@
// Copyright Joyent, Inc. and other Node contributors.
// Copyright Joyent, Inc. and other Node contributors.
Copy link
Member

Choose a reason for hiding this comment

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

Can you undo the whitespace changes?

@bnoordhuis
Copy link
Member

In reference to #17859, I will say that it's still not entirely clear to my what you're doing that necessitates this.

@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

@levimm could you elaborate what this is necessary for?

@BridgeAR
Copy link
Member

BridgeAR commented Feb 6, 2018

Ping @levimm

1 similar comment
@BridgeAR
Copy link
Member

Ping @levimm

@levimm
Copy link
Author

levimm commented Feb 26, 2018

Sorry for the late response, @BridgeAR.
I was trying to create a new node isolate in a thread and register this isolate to the default node platform.

@levimm
Copy link
Author

levimm commented Mar 1, 2018

I realize this doesn't help much to solve my problem. My real issue is PumpMessageLoop is removed from node and I have to include extra v8_libplatform dependency in order to call this method. I'll close this one.

@levimm levimm closed this Mar 1, 2018
@bnoordhuis
Copy link
Member

For posterity, bringing back PumpMessageLoop() is issue #17254.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@bnoordhuis bnoordhuis bnoordhuis left review comments

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@levimm @bnoordhuis @BridgeAR @TimothyGu @nodejs-github-bot