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: reduced env.h and env-inl.h include lines and added forward declarations of class Environment where necessary. #27928

Closed
wants to merge 1 commit into from

Conversation

sarkararpan710
Copy link

Reduces env.h and env-inl.h include lines and added forward declarations of class Environment.
Some files had references to Environment* types in the code but for those files, no include<env.h> or include<env-inl.h> files where present. In those cases I added a forward declaration of class Environment.

Running the vcbuild test on windows ran it successfully.

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

…arations of class Environment where necessary.
@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 May 27, 2019
@addaleax
Copy link
Member

@BridgeAR @sarkararpan710 I don’t think there’s much to review here until the PR compiles & passes linting + tests … for one, the forward declaration of Environment needs to happen inside a namespace node { ... }, but removing the inline header include still won’t work (= won’t compile) in all cases.

@sarkararpan710
Copy link
Author

Yeah I saw that it didn't compile and I am actually looking and exploring methods to potentially speed up the build times using the forward declaration. I guess even first time contributions on open source projects are tough 😂. Nonetheless I would like to know further on how I could help resolve the issue.

@Trott Trott added the wip Issues and PRs that are still a work in progress. label May 30, 2019
@fhinkel
Copy link
Member

fhinkel commented Nov 1, 2019

Should this remain open?

@gireeshpunathil
Copy link
Member

looks like this might have been made redundant through #30133 . Looking at the number of files touched, it is possible that this is still relevant. A rebase followed by a successful compilation can tell us. @sarkararpan710 - are you willing to pursue this further? I can provide some assistance to make progress.

@addaleax
Copy link
Member

addaleax commented Nov 1, 2019

So … I tried rebasing this PR, but it seems like this only adds forward declarations outside of the node namespace (where they can’t be used) and still wouldn’t compile/pass linting because it removes includes that are actually used.

I think closing this makes sense.

@addaleax addaleax closed this Nov 1, 2019
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. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants