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: cleanup unused headers #30328

Closed
wants to merge 8 commits into from
Closed

src: cleanup unused headers #30328

wants to merge 8 commits into from

Conversation

@alferpal
Copy link
Contributor

alferpal commented Nov 7, 2019

Node codebase has evolved a lot in the more than 10 years of its
existence. As more features (and code) have been added, changed,
removed, it's sometimes hard to keep track of what gets used and what
not.

This commits attempts to clean some of those potentially left-over
headers using suggestions from include-what-you-use

Refs: #27531

In addition, also tested with make -j8 make -j8 test-all

namespace v8 {
class Object;
template <class T> class Local;
} // namespace v8

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 7, 2019

Member

This file includes v8.h indirectly anyway, should we add this here? (ditto below?)

Like, any file that actively uses V8 APIs will have it included anyway, through node.h or otherwise…

This comment has been minimized.

Copy link
@alferpal

alferpal Nov 7, 2019

Author Contributor

I'll take a look, remove the declarations and let you know of the result (sometime tomorrow afternoon / evening)

This comment has been minimized.

Copy link
@alferpal

alferpal Nov 8, 2019

Author Contributor

I think I've removed the declarations you pointed, and they were as you said not needed.

Were the ones removed by my last commit all the ones you were referring to?

@mscdex

This comment has been minimized.

Copy link
Contributor

mscdex commented Nov 7, 2019

Isn't it possible to utilize some kind of tooling to automatically check/take care of this sort of thing?

@alferpal

This comment has been minimized.

Copy link
Contributor Author

alferpal commented Nov 8, 2019

@mscdex This PR originates from testing one of such tools (include-what-you-use) against node's c++ codebase.

The problem I've noticed with include-what-you-use is that it works on the assumption that everything used in a file, is going to be explicitly included.

This assumption seems not to be aligned with the reality of node's C++ code, where, as @addaleax pointed in her review comment, some declarations are fine to be implicitly included by includes of your includes.

As for other tools, I've never used C++ professionally / in large codebases and I don't know if they exists, as I'm way outside C++ 's ecosystem.

@nodejs-github-bot

This comment has been minimized.

src/tracing/agent.h Outdated Show resolved Hide resolved
@alferpal

This comment has been minimized.

Copy link
Contributor Author

alferpal commented Nov 10, 2019

In the last CI run, there was a failure in node-test-commit-osx for osx1011 (log here) and this commit tries to fix that.

The other two errors in that CI run, I'm not sure what to make of them.


namespace node {

class Environment;

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 10, 2019

Member

async_wrap.h is guaranteed to have a forward declaration of this because it uses it for the AsyncWrap constructor

namespace v8 {
class Object;
template <class T> class Local;
} // namespace v8

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 10, 2019

Member

stream_wrap.h is guaranteed to include v8.h because it defines a BaseObject subclass

#include "base_object-inl.h"

namespace node {

class Environment;

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 10, 2019

Member

base_object-inl.h is guaranteed to have a forward declaration of this because it uses it as a return type… however, it’s not great that base_object-inl.h is included here rather than base_object.h. Can we replace it with the latter?

@@ -27,7 +27,7 @@
// Decodes a v8::Local<v8::String> or Buffer to a raw char*

#include "v8.h"
#include "env.h"
#include "env-inl.h"

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 10, 2019

Member

Can you explain why this is necessary? If an inline function here uses an inline function from env.h, then it’s probably best to either make it non-inline and move it to the .cc file, or create a string_bytes-inl.h that defines that function and includes env-inl.h?

This comment has been minimized.

Copy link
@alferpal

alferpal Nov 11, 2019

Author Contributor

The more I think of it, it's probably not necessary and an over reaction on my side to this CI error on osx https://ci.nodejs.org/job/node-test-commit-osx/29819/nodes=osx1011/console

Why would only that build fail for that particular reason still leaves me puzzled

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 11, 2019

Member

Yeah… I don’t know either, but I think it makes sense to move StringBytes::InlineDecoder::Decode() into a string_bytes-inl.h?

#include "handle_wrap.h"
#include "uv.h"
#include "v8.h"

namespace node {

class AsyncWrap;

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 10, 2019

Member

handle_wrap.h is guaranteed to provide AsyncWrap because including it defines a subclass of AsyncWrap

@nodejs-github-bot

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 27, 2019

@alferpal Looks like there’s a merge commit in here now … can you rebase that out? Our CI doesn’t handle merge commits well, unfortunately

@alferpal

This comment has been minimized.

Copy link
Contributor Author

alferpal commented Nov 28, 2019

alferpal added 8 commits Nov 7, 2019
Node codebase has evolved a lot in the more than 10 years of its
existence. As more features (and code) have been added, changed,
removed, it's sometimes hard to keep track of what gets used and what
not.

This commits attempts to clean some of those potentially left-over
headers using suggestions from  include-what-you-use

Refs: #27531
Introduced in a previous commit, this delcarations proved to be not
needed.
It seems that the dependency string_bytes had in Environment::isolate()
was always supplied transitively and not directly.

Removing env-inl from api/encoding.cc broke this dependency chain, and
this commit attempts to fix this at the root.
Include the plain header file instead of its inline counterpart
@nodejs-github-bot

This comment has been minimized.

addaleax added a commit that referenced this pull request Nov 28, 2019
Node codebase has evolved a lot in the more than 10 years of its
existence. As more features (and code) have been added, changed,
removed, it's sometimes hard to keep track of what gets used and what
not.

This commits attempts to clean some of those potentially left-over
headers using suggestions from  include-what-you-use

Refs: #27531

PR-URL: #30328
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 28, 2019

Landed in 7686b5a, sorry for all the delays and thanks for the PR! 🎉

@addaleax addaleax closed this Nov 28, 2019
addaleax added a commit that referenced this pull request Nov 30, 2019
Node codebase has evolved a lot in the more than 10 years of its
existence. As more features (and code) have been added, changed,
removed, it's sometimes hard to keep track of what gets used and what
not.

This commits attempts to clean some of those potentially left-over
headers using suggestions from  include-what-you-use

Refs: #27531

PR-URL: #30328
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Dec 1, 2019
Node codebase has evolved a lot in the more than 10 years of its
existence. As more features (and code) have been added, changed,
removed, it's sometimes hard to keep track of what gets used and what
not.

This commits attempts to clean some of those potentially left-over
headers using suggestions from  include-what-you-use

Refs: #27531

PR-URL: #30328
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 2019
Node codebase has evolved a lot in the more than 10 years of its
existence. As more features (and code) have been added, changed,
removed, it's sometimes hard to keep track of what gets used and what
not.

This commits attempts to clean some of those potentially left-over
headers using suggestions from  include-what-you-use

Refs: nodejs#27531

PR-URL: nodejs#30328
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Dec 17, 2019
Node codebase has evolved a lot in the more than 10 years of its
existence. As more features (and code) have been added, changed,
removed, it's sometimes hard to keep track of what gets used and what
not.

This commits attempts to clean some of those potentially left-over
headers using suggestions from  include-what-you-use

Refs: #27531

PR-URL: #30328
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.