src: remove memory_tracker-inl.h from header files by sam-github · Pull Request #27755 · nodejs/node

sam-github · 2019-05-17T20:12:36Z

Inline headers should only be included into the .cc files that use them.

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

nodejs-github-bot · 2019-05-17T20:12:37Z

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3555

nodejs-github-bot · 2019-05-17T21:45:29Z

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

Trott

Looks like this doesn't build as-is?

Trott

Oh, that's probably what the force-push was all about....

sam-github · 2019-05-17T23:25:31Z

yeah, forgot to build test/cctest locally. make all works on my linux now, we'll see about the rest of the platforms.

nodejs-github-bot · 2019-05-18T02:17:52Z

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

addaleax · 2019-05-19T20:26:12Z

I think the OS X failure is real:

In file included from ../src/api/environment.cc:1:
In file included from ../src/env.h:27:
In file included from ../src/aliased_buffer.h:7:
../src/util.h:253:43: error: inline function 'node::ContainerOf<uv_work_s, node::ThreadPoolWork>' is not defined [-Werror,-Wundefined-inline]
constexpr ContainerOfHelper<Inner, Outer> ContainerOf(Inner Outer::*field,
                                          ^
../src/node_internals.h:261:32: note: used here
        ThreadPoolWork* self = ContainerOf(&ThreadPoolWork::work_req_, req);

nodejs-github-bot · 2019-05-22T03:39:05Z

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

The presence of the inline definitions in node_internals.h can cause all files that include node_internals.h to depend on util-inl.h, even if they never use ThreadPoolWork. Whether this happens depends on the toolchain, gcc will strip unused definitions, clang won't.

Inline headers should only be included into the .cc files that use them.

nodejs-github-bot · 2019-05-22T18:25:44Z

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

sam-github · 2019-05-22T19:27:53Z

OK, I sorted out the clang vs gcc disagreement on whether unused inlines need to be defined, and finally have a green CI.

Because I force-pushed since last review, I need one reviewer to reaffirm their approval, please, before I can land this.

sam-github · 2019-05-23T15:52:47Z

Landed in cb16872...60b315c

The presence of the inline definitions in node_internals.h can cause all files that include node_internals.h to depend on util-inl.h, even if they never use ThreadPoolWork. Whether this happens depends on the toolchain, gcc will strip unused definitions, clang won't. PR-URL: #27755 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Inline headers should only be included into the .cc files that use them. PR-URL: #27755 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

The presence of the inline definitions in node_internals.h can cause all files that include node_internals.h to depend on util-inl.h, even if they never use ThreadPoolWork. Whether this happens depends on the toolchain, gcc will strip unused definitions, clang won't. PR-URL: #27755 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Inline headers should only be included into the .cc files that use them. PR-URL: #27755 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

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. labels May 17, 2019

lpinca approved these changes May 17, 2019

View reviewed changes

Trott approved these changes May 17, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 17, 2019

cjihrig approved these changes May 17, 2019

View reviewed changes

sam-github force-pushed the clean-memory-tracker-inl branch 2 times, most recently from 1dee3ab to f79a88c Compare May 17, 2019 23:19

Trott requested changes May 17, 2019

View reviewed changes

Trott approved these changes May 17, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

addaleax approved these changes May 19, 2019

View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 19, 2019

jasnell approved these changes May 19, 2019

View reviewed changes

sam-github force-pushed the clean-memory-tracker-inl branch 5 times, most recently from 63762f0 to ea6ff98 Compare May 21, 2019 22:58

sam-github added 2 commits May 22, 2019 09:50

src: remove memory_tracker-inl.h from header files

ad131e2

Inline headers should only be included into the .cc files that use them.

sam-github force-pushed the clean-memory-tracker-inl branch from ea6ff98 to a1053e2 Compare May 22, 2019 16:50

src: remove env-inl.h from header files

0af62cc

Inline headers should only be included into the .cc files that use them.

sam-github force-pushed the clean-memory-tracker-inl branch from a1053e2 to 0af62cc Compare May 22, 2019 18:23

sam-github mentioned this pull request May 22, 2019

cleanup: reduce the number of #include "env.h" and #include "env-inl.h" in the code base #27531

Closed

sam-github closed this May 23, 2019

sam-github deleted the clean-memory-tracker-inl branch May 23, 2019 15:52

targos mentioned this pull request Jun 3, 2019

v12.4.0 release proposal #28040

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

src: remove memory_tracker-inl.h from header files#27755

src: remove memory_tracker-inl.h from header files#27755
sam-github wants to merge 3 commits intonodejs:masterfrom
sam-github:clean-memory-tracker-inl

sam-github commented May 17, 2019

Uh oh!

nodejs-github-bot commented May 17, 2019

Uh oh!

nodejs-github-bot commented May 17, 2019

Uh oh!

Trott left a comment

Uh oh!

Trott left a comment

Uh oh!

sam-github commented May 17, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented May 18, 2019

Uh oh!

addaleax commented May 19, 2019

Uh oh!

nodejs-github-bot commented May 22, 2019

Uh oh!

nodejs-github-bot commented May 22, 2019

Uh oh!

sam-github commented May 22, 2019

Uh oh!

sam-github commented May 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

sam-github commented May 17, 2019

Checklist

Uh oh!

nodejs-github-bot commented May 17, 2019

Uh oh!

nodejs-github-bot commented May 17, 2019

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github commented May 17, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented May 18, 2019

Uh oh!

addaleax commented May 19, 2019

Uh oh!

nodejs-github-bot commented May 22, 2019

Uh oh!

nodejs-github-bot commented May 22, 2019

Uh oh!

sam-github commented May 22, 2019

Uh oh!

sam-github commented May 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants