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: use stack-allocated Environment instances #7090

Merged
merged 2 commits into from
Jun 2, 2016

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jun 1, 2016

@bnoordhuis bnoordhuis 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 Jun 1, 2016
@Fishrock123
Copy link
Contributor

Fishrock123 commented Jun 1, 2016

(Comment is at #7082 (comment))

Curious, does this buy us anything in reality? Just less management of environment?

Seems fine to me, not going to sign-off though. (CI is green fwiw)

@bnoordhuis
Copy link
Member Author

Curious, does this buy us anything in reality? Just less management of environment?

It makes it obvious that the instance's lifetime is correctly scoped, something you don't have with heap-allocated instances.

@jasnell
Copy link
Member

jasnell commented Jun 1, 2016

Seems like a good idea. Scanning through it the one thing that does come to mind is that it would be helpful to have additional code comments added as these kinds of changes are being made... comments that help explain the flow and intent more so that contributors who are less familiar with the code can grok things a bit easier. Beyond that nothing else comes to mind. LGTM given that CI is green. Would like @trevnorris to review also tho.

@bnoordhuis
Copy link
Member Author

comments that help explain the flow and intent more so that contributors who are less familiar with the code can grok things a bit easier.

Can you point out a few places where comments would help? In general I try to refrain from comments unless the code does something that's not the obviously correct thing to do but I don't think that's an issue here.

@jasnell
Copy link
Member

jasnell commented Jun 1, 2016

I'm thinking in terms of general comments that would help someone who is new to node understand what is happening within the code. For instance, a quick one liner that explains what InitAdapter() does (https://github.com/nodejs/node/pull/7090/files#diff-883701a662f7ab4f6396a4d5fbb1149cR187). Obviously not a critical thing by any measure, I'm just thinking in terms of ways to make the code more accessible.

@bnoordhuis
Copy link
Member Author

We used to have such comments in src/node.cc and src/node.js but they have a habit of getting out of sync with the code.

As to your particular example, that's existing code that this PR doesn't fundamentally change. I don't think this PR is the right place to start commenting it, that's mixing two different things.

@jasnell
Copy link
Member

jasnell commented Jun 1, 2016

I don't disagree :-) I was just using it as an example. No worries :-)

#include "v8.h"
#include "v8-profiler.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Which header is it that includes SetupProcessObject()?

Copy link
Member Author

Choose a reason for hiding this comment

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

node_internals.h, which is included by node.h, which is included by env-inl.h. It's a bit roundabout, I can include it directly.

Copy link
Contributor

@trevnorris trevnorris Jun 2, 2016

Choose a reason for hiding this comment

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

doesn't worry me. just couldn't see how the header was reaching here.

@trevnorris
Copy link
Contributor

Like seeing code moved out of node.cc. LGTM.

PR-URL: nodejs#7090
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Makes it easier to reason about the lifetime of the Environment object.

PR-URL: nodejs#7090
Refs: nodejs#7082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@bnoordhuis bnoordhuis closed this Jun 2, 2016
@bnoordhuis bnoordhuis deleted the stack-allocated-env branch June 2, 2016 20:41
@bnoordhuis bnoordhuis merged commit aac79df into nodejs:master Jun 2, 2016
@evanlucas
Copy link
Contributor

This is not landing cleanly, so holding off on including it in the next v6 release.

@Fishrock123
Copy link
Contributor

Still conflicts to v6, with or without the inspector, and the inspector depends on it... Going to patch without; I think that conflict is easier to resolve.

@Fishrock123
Copy link
Contributor

Looks like this has a hard dependency on 138c7af / #7082, conflict resolution does not appear possible from my end.

@MylesBorins
Copy link
Contributor

@bnoordhuis safe to assume dont land on v4.x as well?

@bnoordhuis
Copy link
Member Author

Correct. I've added the label.

@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
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.

6 participants