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: Fix extra includes of "env.h" and "env-inl.h" in this directory. #32293

Closed
wants to merge 3 commits into from

Conversation

nkreeger
Copy link
Contributor

This commit should cleanup the remaining open extra includes of "env.h"
and "env-inl.h" and forward declarations of |Environment|.

Fixes: #27531

Checklist
  • [x ] make -j4 test (UNIX), or vcbuild test (Windows) passes
  • [ x] commit message follows commit guidelines

@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. labels Mar 16, 2020
Copy link
Member

@himself65 himself65 left a comment

Choose a reason for hiding this comment

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

Commit message should be like 3bb085d Or 902f5d9

src: change env.h includes for forward declarations
Due to how the Environment class is used through the codebase,
there are a lot of includes referencing either env.h or env-inl.h.
This can cause that when any development touches those libraries,
a lot of files have to be recompiled.
This commit attempts to change those includes by forward declarations
when possible to mitigate the issue.

Due to how the Environment class is used throughout the codebase, there
are a log of includes referencing eitehr env.h or env-inl.h.

This commit cleans up the remaining extra includes of 'env.h' or
'env-inl.h' and adds forward declarations of the Environment class.

PR-URL: nodejs#32293
Refs: nodejs#27531
@nkreeger
Copy link
Contributor Author

Commit message should be like [3bb085d]
Thanks updated - please let me know if I did something wrong again.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

👍

src/node_crypto_common.h Outdated Show resolved Hide resolved
@@ -4,7 +4,6 @@
#include "node_http_common.h"
#include "node.h"
#include "node_mem-inl.h"
#include "env-inl.h"
Copy link
Member

Choose a reason for hiding this comment

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

If removing this works, then it’s by accident – this file does require env-inl.h in order to work. Please leave the include here.

(You can test this by including only node_http_common-inl.h from a .cc file – compiling that file will fail without this line.)

Copy link
Member

Choose a reason for hiding this comment

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

I can guarantee that removing this would break on some of the platforms as I had omitted this originally and had to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great thanks - any tips on better build testing? I git grep'ing for these items in header files and running make -j8 test as a blanket test. I see the x-platform integration tests pass...

@jasnell Please let me know if I'm stomping on anything you're doing. Just looking for some fun side-hacking :-)

Copy link
Member

Choose a reason for hiding this comment

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

Nope you're good :-) ... I'll adjust on my end if there are any merge conflicts.

src/stream_base-inl.h Show resolved Hide resolved
@nkreeger
Copy link
Contributor Author

@addaleax @jasnell updated - PTAL.

@nkreeger
Copy link
Contributor Author

Friendly ping on this - any other reviewers I should include?

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 29, 2020
@addaleax
Copy link
Member

@nkreeger Thanks for the ping! I’ll kick off a fresh CI run and we can land this quite soon, probably.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 2, 2020

addaleax pushed a commit that referenced this pull request Apr 2, 2020
Due to how the Environment class is used throughout the codebase, there
are a log of includes referencing eitehr env.h or env-inl.h.

This commit cleans up the remaining extra includes of 'env.h' or
'env-inl.h' and adds forward declarations of the Environment class.

PR-URL: #32293
Refs: #27531

Fixes: #27531
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
Copy link
Member

addaleax commented Apr 2, 2020

Landed in 12a2997, and thank you for your patience! 🎉

@addaleax addaleax closed this Apr 2, 2020
@nkreeger nkreeger deleted the kreeger/include-fixes branch April 3, 2020 03:12
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
Due to how the Environment class is used throughout the codebase, there
are a log of includes referencing eitehr env.h or env-inl.h.

This commit cleans up the remaining extra includes of 'env.h' or
'env-inl.h' and adds forward declarations of the Environment class.

PR-URL: #32293
Refs: #27531

Fixes: #27531
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Apr 12, 2020
Due to how the Environment class is used throughout the codebase, there
are a log of includes referencing eitehr env.h or env-inl.h.

This commit cleans up the remaining extra includes of 'env.h' or
'env-inl.h' and adds forward declarations of the Environment class.

PR-URL: #32293
Refs: #27531

Fixes: #27531
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos added backport-blocked-v12.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-blocked-v12.x labels Apr 22, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Due to how the Environment class is used throughout the codebase, there
are a log of includes referencing eitehr env.h or env-inl.h.

This commit cleans up the remaining extra includes of 'env.h' or
'env-inl.h' and adds forward declarations of the Environment class.

PR-URL: nodejs#32293
Refs: nodejs#27531

Fixes: nodejs#27531
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
Due to how the Environment class is used throughout the codebase, there
are a log of includes referencing eitehr env.h or env-inl.h.

This commit cleans up the remaining extra includes of 'env.h' or
'env-inl.h' and adds forward declarations of the Environment class.

PR-URL: #32293
Refs: #27531

Fixes: #27531
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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