Skip to content

src: add include guards to internal headers - #6948

Merged
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:internal-include-guards
May 25, 2016
Merged

src: add include guards to internal headers#6948
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:internal-include-guards

Conversation

@bnoordhuis

@bnoordhuisbnoordhuis commented May 24, 2016

Copy link
Copy Markdown
Member

For consistency with the newly added src/base64.h header, check that
NODE_WANT_INTERNALS is defined and set in internal headers.

Refs: #6910
CI: https://ci.nodejs.org/job/node-test-pull-request/2758/

R=@indutny@trevnorris

@bnoordhuisbnoordhuis added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 24, 2016
@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 24, 2016
@trevnorris

Copy link
Copy Markdown
Contributor

LGTM. Should this be documented in our addons docs?

@bnoordhuis

Copy link
Copy Markdown
MemberAuthor

What in particular does 'this' refer to?

@trevnorris

Copy link
Copy Markdown
Contributor

@bnoordhuis In all of test/addons there's an added 'NODE_WANT_INTERNALS=1' and was wondering if that gyp option should be documented somewhere.

@bnoordhuis

Copy link
Copy Markdown
MemberAuthor

I only added it to the binding.gyp files from add-ons that #include "util.h" (because they use CHECK.)

As to documenting NODE_WANT_INTERNALS, it's expressly not the intent that third-party add-ons can include internal files. =)

@trevnorris

Copy link
Copy Markdown
Contributor

Heh, fair enough.

@mscdexmscdex removed the lib / src Issues and PRs related to general changes in the lib or src directory. label May 24, 2016
@Fishrock123

Copy link
Copy Markdown
Contributor

Isn't this kind of breaking?

@bnoordhuis

Copy link
Copy Markdown
MemberAuthor

Only on smartos, apparently.

Jest aside, all of these files are internal - people are not supposed to be using them and if they do, they should expect things to break at any time. The installer doesn't install them.

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

@trevnorris

trevnorris commented May 24, 2016

Copy link
Copy Markdown
Contributor

The installer doesn't install them.

Can you tell me why this compiles fine (tested on v4 and v6)?

#include<v8.h>
#include<async-wrap.h>
#include<node.h>namespacebmod {
using v8::FunctionCallbackInfo;
using v8::Value;
using v8::Local;
using v8::Object;
voidRun(const FunctionCallbackInfo<Value>& args) {
fprintf(stderr, "NODE_ASYNC_ID_OFFSET: %i\n", NODE_ASYNC_ID_OFFSET);
}
voidInit(Local<Object> exports) {
NODE_SET_METHOD(exports, "run", Run);
}
} // namespace bmodNODE_MODULE(addon, bmod::Init)

@bnoordhuis

bnoordhuis commented May 24, 2016

Copy link
Copy Markdown
MemberAuthor

When I say installer, I mean tools/install.py. I don't know what the OS X or Windows installers do but the binary tarballs don't include internal headers.

If you use node-gyp though, that downloads and builds against the source tarball, and that does include internal headers.

@jasnell

Copy link
Copy Markdown
Member

LGTM if CI is green

@indutny

Copy link
Copy Markdown
Member

LGTM

For consistency with the newly added src/base64.h header, check that
NODE_WANT_INTERNALS is defined and set in internal headers.
PR-URL: nodejs#6948
Refs: nodejs#6910
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis
bnoordhuisforce-pushed the internal-include-guards branch from c79a00d to eff96d3CompareMay 25, 2016 07:57
@bnoordhuis
bnoordhuis deleted the internal-include-guards branch May 25, 2016 07:57
@bnoordhuis
bnoordhuis merged commit eff96d3 into nodejs:masterMay 25, 2016
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
For consistency with the newly added src/base64.h header, check that
NODE_WANT_INTERNALS is defined and set in internal headers.
PR-URL: nodejs#6948
Refs: nodejs#6910
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
For consistency with the newly added src/base64.h header, check that
NODE_WANT_INTERNALS is defined and set in internal headers.
PR-URL: #6948
Refs: #6910
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@MylesBorins

MylesBorins commented Jul 11, 2016

Copy link
Copy Markdown
Contributor

@bnoordhuis lts?

@bnoordhuis

Copy link
Copy Markdown
MemberAuthor

Let's not.

Sign up for freeto 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.

8 participants

@bnoordhuis@trevnorris@Fishrock123@jasnell@indutny@MylesBorins@mscdex@nodejs-github-bot