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: avoid std::make_unique #20386

Closed
wants to merge 1 commit into from
Closed

Conversation

addaleax
Copy link
Member

Work around nodejs/build#1254, which
effectively breaks stress test CI and CITGM, by avoiding
std::make_unique for now.

This workaround should be reverted once that issue is resolved.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Work around nodejs/build#1254, which
effectively breaks stress test CI and CITGM, by avoiding
`std::make_unique` for now.

This workaround should be reverted once that issue is resolved.

Refs: nodejs/build#1254
@addaleax addaleax 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. fast-track PRs that do not need to wait for 48 hours to land. labels Apr 28, 2018
@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 Apr 28, 2018
@addaleax
Copy link
Member Author

Wasn’t sure about whether to open this PR, but I have no idea how soon the build issue is resolved and I think we want at least CITGM to be available for now – please 👍 this comment if you agree with fast-tracking.

CI: https://ci.nodejs.org/job/node-test-pull-request/14570/

@addaleax
Copy link
Member Author

addaleax commented Apr 28, 2018

Here’s a re-run of the stress test CI that I originally wanted to run, to be sure:

Stress CI (master): https://ci.nodejs.org/job/node-stress-single-test/1827/
Stress CI (this PR): https://ci.nodejs.org/job/node-stress-single-test/1828/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 29, 2018
@addaleax
Copy link
Member Author

Landed in 283a967

@addaleax addaleax closed this Apr 29, 2018
@addaleax addaleax deleted the no-make-unique branch April 29, 2018 21:46
addaleax added a commit that referenced this pull request Apr 29, 2018
Work around nodejs/build#1254, which
effectively breaks stress test CI and CITGM, by avoiding
`std::make_unique` for now.

This workaround should be reverted once that issue is resolved.

Refs: nodejs/build#1254

PR-URL: #20386
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 4, 2018
Work around nodejs/build#1254, which
effectively breaks stress test CI and CITGM, by avoiding
`std::make_unique` for now.

This workaround should be reverted once that issue is resolved.

Refs: nodejs/build#1254

PR-URL: #20386
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
addaleax added a commit to addaleax/node that referenced this pull request Oct 24, 2018
This broke Travis CI. We have run into this problem with
`std::make_unique` in the past, and opted to not use it
for now, e.g. in nodejs#20386.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 48 hours to land. 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.

5 participants