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: don't crash with no arguments to print #106

Merged
merged 4 commits into from
Jun 29, 2017
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jun 23, 2017

This commit prevents a segfault on v8 print.

I'll be adding a test as well, but it requires some changes to test/common.js first.

bnoordhuis

This comment was marked as off-topic.

This commit prevents the DoExecute() methods from segfaulting
when the cmd argument is null.
@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 24, 2017

Replaced the remaining uses of NULL and updated the other commands in a similar fashion. I've got four test cases to add, but have to update the test Session to expose stderr first.

This commit allows stderr to be scripted in the same way that
stdout is.
@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 24, 2017

OK, I allowed stderr to be scripted the same way that stdout currently is. Then, I added a test for the usage messages. Those commands segfault on master, and work correctly with the fixes in this PR.

bnoordhuis

This comment was marked as off-topic.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 28, 2017

@bnoordhuis what is the policy for PRs getting landed in this repo? I technically have commit rights and can land this myself, but I don't want to step on any toes either.

@bnoordhuis
Copy link
Member

Go ahead and land it, no toes to worry about.

@hhellyer
Copy link
Contributor

@bnoordhuis @cjihrig - I can land things but I'm not sure whether I should be able to in a Node.js repo or whether my access ended up being grandfathered in when @indutny moved the repo from his space to the nodejs org. So I don't usually unless the PR has had a couple of LGTMs and been waiting a little while!
Things are building up though - I'm happy to clean up the backlog if you guys want.

@bnoordhuis
Copy link
Member

@hhellyer Go for it! :-)

@hhellyer hhellyer merged commit 628ad41 into nodejs:master Jun 29, 2017
@cjihrig cjihrig deleted the print branch July 1, 2017 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants