Skip to content

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Nov 17, 2016

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 17, 2016
@mscdex mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 17, 2016
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

Just curious, is this making any problems right now?

@bnoordhuis
Copy link
Member Author

No, not unless you run valgrind.

Make cctest valgrind-clean again by freeing heap-allocated memory.
Overlooked in commit ea94086 ("src: provide allocation + nullptr check
shortcuts.")

PR-URL: nodejs#9667
Refs: nodejs#8482
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
@bnoordhuis bnoordhuis closed this Nov 18, 2016
@bnoordhuis bnoordhuis deleted the cctest-valgrind branch November 18, 2016 20:59
@bnoordhuis bnoordhuis merged commit b80f05e into nodejs:master Nov 18, 2016
addaleax pushed a commit that referenced this pull request Nov 22, 2016
Make cctest valgrind-clean again by freeing heap-allocated memory.
Overlooked in commit ea94086 ("src: provide allocation + nullptr check
shortcuts.")

PR-URL: #9667
Refs: #8482
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
@MylesBorins
Copy link
Contributor

@bnoordhuis should we backport this?

@addaleax
Copy link
Member

@thealphanerd I don’t think this applies to v4.x, but for v6 yes

@MylesBorins
Copy link
Contributor

@addaleax this does not land cleanly on v6, manual backport?

@MylesBorins
Copy link
Contributor

ping @addaleax

@addaleax
Copy link
Member

adding dont-land-on-v6.x because it’s really not worth the trouble, I’d say

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++. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants