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 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

@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 Nov 7, 2019
namespace v8 {
class Object;
template <class T> class Local;
} // namespace v8
Copy link
Member

Choose a reason for hiding this comment

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

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…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@alferpal alferpal Nov 8, 2019

Choose a reason for hiding this comment

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

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
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
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
Copy link
Collaborator

src/tracing/agent.h Outdated Show resolved Hide resolved
@alferpal
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;
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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

#include "base_object-inl.h"

namespace node {

class Environment;
Copy link
Member

Choose a reason for hiding this comment

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

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"
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

src/udp_wrap.h Outdated
#include "handle_wrap.h"
#include "uv.h"
#include "v8.h"

namespace node {

class AsyncWrap;
Copy link
Member

Choose a reason for hiding this comment

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

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 18, 2019
@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

@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
Copy link
Contributor Author

alferpal commented Nov 28, 2019 via email

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
Copy link
Collaborator

addaleax pushed 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
Copy link
Member

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

@addaleax addaleax closed this Nov 28, 2019
addaleax pushed 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 pushed 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
MylesBorins pushed 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
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

7 participants