Skip to content

Conversation

@levimm
Copy link

@levimm levimm commented Jan 2, 2018

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

src/node.h Outdated
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
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.

@levimm levimm force-pushed the export-default-platform branch from 4204a26 to a37abb5 Compare January 3, 2018 03:05
@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

Labels

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.

5 participants