FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: use stack-allocated Environment instances by bnoordhuis · Pull Request #7090 · nodejs/node · GitHub

/ node Public

src: use stack-allocated Environment instances - #7090

Merged
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:stack-allocated-env
Jun 2, 2016
ConversationCommits (2)ChecksFiles changed
Merged

src: use stack-allocated Environment instances#7090
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:stack-allocated-env

Conversation

bnoordhuis commented Jun 1, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Jun 1, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

(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 commented Jun 1, 2016

Copy link
Copy Markdown
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 commented Jun 1, 2016

Copy link
Copy Markdown
Member

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 commented Jun 1, 2016

Copy link
Copy Markdown
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 commented Jun 1, 2016

Copy link
Copy Markdown
Member

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 commented Jun 1, 2016

Copy link
Copy Markdown
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 commented Jun 1, 2016

Copy link
Copy Markdown
Member

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

trevnorris reviewed Jun 1, 2016
View reviewed changes
Comment thread src/env.cc Outdated

trevnorris Jun 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Which header is it that includes SetupProcessObject()?

bnoordhuis Jun 2, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

trevnorris Jun 2, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

trevnorris commented Jun 1, 2016

Copy link
Copy Markdown
Contributor

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

bnoordhuis added 2 commits June 2, 2016 19:38
src: move env init logic into Environment class
58cec4e 
PR-URL: nodejs#7090
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
src: use stack-allocated Environment instances
aac79df 
Makes it easier to reason about the lifetime of the Environment object.

PR-URL: nodejs#7090
Refs: nodejs#7082
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
bnoordhuis force-pushed the stack-allocated-env branch from e4563dc to aac79df Compare June 2, 2016 20:40
bnoordhuis closed this Jun 2, 2016
bnoordhuis deleted the stack-allocated-env branch June 2, 2016 20:41
bnoordhuis merged commit aac79df into nodejs:master Jun 2, 2016

evanlucas commented Jun 16, 2016

Copy link
Copy Markdown
Contributor

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

Fishrock123 commented Jul 5, 2016

Copy link
Copy Markdown
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 commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

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

MylesBorins mentioned this pull request Jul 11, 2016
Closed

MylesBorins commented Jul 11, 2016

Copy link
Copy Markdown
Contributor

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

bnoordhuis commented Jul 11, 2016

Copy link
Copy Markdown
Member Author

Correct. I've added the label.

gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL