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.

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

src: move trace_event.h include to internal header #10959

Merged
merged 1 commit into from
Jan 26, 2017

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jan 23, 2017

Move the include from src/node.h to src/node_internals.h.

trace_event.h is not shipped in binary-only and headers-only tarballs,
making it currently impossible to build add-ons against them.

cc @richardlau @matthewloring

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v7.x labels Jan 23, 2017
@matthewloring
Copy link

LGTM

1 similar comment
@richardlau
Copy link
Member

LGTM

@joshgav
Copy link
Contributor

joshgav commented Jan 23, 2017

Seems this would make the tracing macros unavailable to native module authors, which I don't think is what we want at least long term. Would it be better to bundle the header?

/cc @nodejs/diagnostics

@matthewloring
Copy link

@joshgav I think it would be a good idea to hold off on encouraging native module authors to use these macros directly. Many of the macros are "implementation details" of other macros and not intended to be used outside the file. We should provide a simplified API for recording trace events or provide documentation on the set of macros intended to be part of the public API.

Copy link
Member

@Qard Qard left a comment

Choose a reason for hiding this comment

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

Agreed that we should probably not be exposing the full surface area of the trace_event.h macros. I think it's best we make this fully internal for now and expose a friendlier API to userland in the future.

@joshgav
Copy link
Contributor

joshgav commented Jan 23, 2017

@Qard @matthewloring

We should provide a simplified API

expose a friendlier API to userland in the future

Makes sense and I agree in principal, although in the discussion at the last Collab Summit we said we wouldn't wrap the macros or provide an alternate C/C++ API.

@matthewloring
Copy link

or provide documentation on the set of macros intended to be part of the public API.

I would also be fine with this approach. Wrapping will incur some performance overhead which we could avoid.

@Qard
Copy link
Member

Qard commented Jan 23, 2017

My only issue with leaving it publicly accessible is that people may try to use it in ways not intended or expected. The _headers issue is a good example of that behaviour. Probably not as problematic as _headers was as this is native and in a less popular component, but we should at least have a plan for how to deal with userland exposure, intentional or otherwise.

Move the include from src/node.h to src/node_internals.h.

trace_event.h is not shipped in binary-only and headers-only tarballs,
making it currently impossible to build add-ons against them.

PR-URL: nodejs#10959
Refs: nodejs/citgm#226 (comment)
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matthew Loring <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@bnoordhuis bnoordhuis closed this Jan 26, 2017
@bnoordhuis bnoordhuis deleted the fix10929 branch January 26, 2017 00:56
@bnoordhuis bnoordhuis merged commit bfd3c7e into nodejs:master Jan 26, 2017
@mscdex mscdex added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Jan 29, 2017
targos pushed a commit to targos/node that referenced this pull request Mar 1, 2017
Move the include from src/node.h to src/node_internals.h.

trace_event.h is not shipped in binary-only and headers-only tarballs,
making it currently impossible to build add-ons against them.

PR-URL: nodejs#10959
Refs: nodejs/citgm#226 (comment)
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matthew Loring <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 1, 2017
Move the include from src/node.h to src/node_internals.h.

trace_event.h is not shipped in binary-only and headers-only tarballs,
making it currently impossible to build add-ons against them.

PR-URL: nodejs#10959
Refs: nodejs/citgm#226 (comment)
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matthew Loring <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@italoacasas italoacasas mentioned this pull request Mar 1, 2017
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++. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants