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

[backport 11.x]: src: move public C++ APIs into src/api/*.cc #26028

Closed
wants to merge 2 commits into from
Closed

[backport 11.x]: src: move public C++ APIs into src/api/*.cc #26028

wants to merge 2 commits into from

Conversation

antsmartian
Copy link
Contributor

Backport: #25541

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

@joyeecheung @antsmartian
src: move v8_platform implementation into node_v8_platform-inl.h
d24da95 
So that the v8_platform global variable can be referenced in other
files.

PR-URL: nodejs#25541
Reviewed-By: Gus Caplan <[email protected]>
@joyeecheung @antsmartian
src: move public C++ APIs into src/api/*.cc
b2682b5 
This patch moves most of the public C++ APIs into src/api/*.cc
so that it's easier to tell that we need to be careful about
the compatibility of these code.

Some APIs, like `node::LoadEnvironmet()`, `node::Start()` and
`node::Init()` still stay in `node.cc` because they are still
very specific to our use cases and do not work quite well yet
for embedders anyway - we could not even manage to write cctest for
them at the moment.

PR-URL: nodejs#25541
Reviewed-By: Gus Caplan <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

@antsmartian build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2560/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. v11.x labels Feb 10, 2019
@antsmartian
Copy link
Contributor Author

Hmmm, this is failing with compilation error:

../src/api/environment.cc:138:8: error: no member named 'ProcessCliArgs' in 'node::Environment'
  env->ProcessCliArgs(args, exec_args);
  ~~~  ^
../src/api/environment.cc:187:43: error: 'LookupAndCompile' is a private member of 'node::native_module::NativeModuleLoader'
        per_process::native_module_loader.LookupAndCompile(
                                          ^
../src/node_native_module.h:93:32: note: declared private here
  v8::MaybeLocal<v8::Function> LookupAndCompile(
                               ^
../src/js_native_api_v8.cc:2156:45: warning: 'ToBoolean' is deprecated: Use maybe version [-Wdeprecated-declarations]
    v8impl::V8LocalValueFromJsValue(value)->ToBoolean(isolate);
                                            ^
../deps/v8/include/v8.h:2533:3: note: 'ToBoolean' has been explicitly marked deprecated here
  V8_DEPRECATED("Use maybe version",
  ^
../deps/v8/include/v8config.h:326:29: note: expanded from macro 'V8_DEPRECATED'
  declarator __attribute__((deprecated(message)))
                            ^
2 errors generated.

@ZYSzys
Copy link
Member

ZYSzys commented Feb 10, 2019

It seems that #25667 should be backported before this.

@antsmartian
Copy link
Contributor Author

@ZYSzys Yup, seems to be the case. Let me see if I can raise a PR for the same..

@targos
Copy link
Member

targos commented Feb 10, 2019

The original commits landed cleanly after backporting #25667

@targos targos closed this Feb 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
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.
5 participants
@antsmartian @nodejs-github-bot @ZYSzys @targos @joyeecheung