Repository navigation
cleanup: reduce the number of #include "env.h" and #include "env-inl.h" in the code base #27531
Description
Activity
I think this is a good first issue if you know how forward declaration and C++ headers work. If nobody picks this up (say in a week) I'll do it anyway because...I touch
env.htoo much these days and want to spend less time waiting for recompilation ;)if it is not in a hot path (i.e. it does not need to be inlined).
BTW I believe that as of C++11 this assumption is not as relevant as it used to be.
Language-wiseinlinehas come to mean "may have multiple definitions" on the one hand, and compilers are now inlining anything and do LTO if they think it will improve performance, on the other hand.I think the instructions from @joyeecheung are very clear. Can I take up this issue? Thanks!
@Jinshuo1994 sure, go head!
Hi, This is my first time with contributing to open source and would like to know, how exactly I can help.
Is this issue still open? Any build instructions that might help me understand how I can contribute.
Thanks.@sarkararpan710 There was #27620 but it did not exactly do what this issue requests. Since there has been no progress there since more than a week, I think you can take another stab at this - please take a look at #27620 (comment) about what the request is.
@joyeecheung, sorry about the delay to update the PR. Been busy w. work lately. I’ll close the PR, and look out for other opportunities to do PRs.
@joyeecheung I was wondering if I could work on this issue, is it still something that needs to be worked on? I am brand new to open-source and this would be my first contribution, so please excuse if I ask something ignorant.
I suggest waiting until #27755 lands. It probably (mostly) cleans up many unnecessary includes of env-inl.h, but it doesn't deal with the problem of files that include env.h purely for its class declaration.
As a heads up, to do this, you'll need to know how a
class Environment;forward-declaration can be used to replace some instance of#include "env.h".@sam-github Thank you very much. Will this take about 48 hours? How will I know when it lands? I appreciate your comment.
Hopefully will land by the end of day. You can probably use the 'subscribe' button on the PR to start getting updates on it.
@sam-github Are we waiting for someone with write access to merge it with Master, am I understanding? Then, I re-build and start working? Thank you. Thank you for your patience with a new person, I am in my last quarter of a CS degree and new to git.
13 remaining items
- added 2 commits that reference this issue
on Nov 28, 2019 - added a commit that references this issue
on Dec 17, 2019 - added a commit that references this issue
on Apr 7, 2020 - added a commit that references this issue
on Apr 12, 2020 - added 2 commits that reference this issue
on Apr 25, 2020 - added a commit that references this issue
on Jul 27, 2026
Because of how
Environmentis used in the code base (e.g. to grab Node.js things from callbacks, to have fast access to persistent properties),Environmentis used in almost every C++ file, which results in a lot of#include "env.h"and#include "env-inl.h"in the code base. However many files (mostly headers) only need a forward declaration ofclass Environmentinstead of requiringenv.horenv-inl.hbecause they just need to know theEnvironmenttype to haveEnvironment*in the declarations. When there is code in a header that actually uses something fromEnvironment(so a forward declaration is not enough), the code may be moved out of the header if it is not in a hot path (i.e. it does not need to be inlined).Reducing the number of these includes should reduce the amount of files being compiled whenever
env.h/env-inl.his touched.