Skip to content

Conversation

@richardlau
Copy link
Member

@richardlaurichardlau commented Feb 22, 2019

Fixes the following build warning:

CXX(target) /home/travis/build/nodejs/node/out/Release/obj.target/node_lib/src/node_report.o ../src/node_report.cc: In function ‘std::string report::TriggerNodeReport(v8::Isolate*, node::Environment*, const char*, const char*, std::string, v8::Local<v8::String>)’: ../src/node_report.cc:184:76: warning: ‘outstream’ may be used uninitialized in this function [-Wmaybe-uninitialized] isolate, env, message, location, filename, out, stackstr, &tm_struct); ^ ../src/node_report.cc:149:17: note: ‘outstream’ was declared here std::ostream* outstream; ^ 
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@richardlau sadly an error occured when I tried to trigger a build :(

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 22, 2019
@richardlaurichardlau added the report Issues and PRs related to process.report. label Feb 22, 2019
@richardlau
Copy link
MemberAuthor

CI will need to wait until the lockdown ends.

@Trott
Copy link
Member

CI looks quiescent right now, so here you go: https://ci.nodejs.org/job/node-test-pull-request/20971/

Not sure if the GitHub widget will still report results or if someone with access will have to let you know if it passed or not.

@richardlau
Copy link
MemberAuthor

CI looks quiescent right now, so here you go: https://ci.nodejs.org/job/node-test-pull-request/20971/

Not sure if the GitHub widget will still report results or if someone with access will have to let you know if it passed or not.

The status is being posted, so the widget is showing results. node-test-commit-windows-fanned failed -- if anyone has access could comment on whether the failure looks related to this PR (in which case please post the relevant log) or some flake/infra issue.

FTR I'm in no particular rush to land this.

@Trott
Copy link
Member

Trott commented Feb 22, 2019

@richardlau The Windows failure is a pervasive infra-related thing. If you're in no rush, once it's fixed, we can do a quick Resume Build.

@Trott
Copy link
Member

I think @refack fixed stuff. 🎉 Here's a Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20972/

@richardlaurichardlauforce-pushed the uninitializedoutstream branch from 1fc7bb9 to ec6420cCompareFebruary 22, 2019 23:40
@richardlau
Copy link
MemberAuthor

Rebased based on irc chat suggestions that the Windows issue is #25593 and that recent commits to master may have mitigated it.

@Trott
Copy link
Member

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

I don't think it's been fixed yet, but I'll be happy to be wrong....

Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

@richardlau
Copy link
MemberAuthor

richardlau commented Feb 28, 2019

@richardlaurichardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 28, 2019
Fixes `maybe-uninitialized` build warning in `src/node_report.cc`. PR-URL: nodejs#26265 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@richardlaurichardlauforce-pushed the uninitializedoutstream branch from ec6420c to 4a10ce6CompareFebruary 28, 2019 16:32
@richardlaurichardlau merged commit 4a10ce6 into nodejs:masterFeb 28, 2019
@richardlau
Copy link
MemberAuthor

Landed in 4a10ce6.

addaleax pushed a commit that referenced this pull request Mar 1, 2019
Fixes `maybe-uninitialized` build warning in `src/node_report.cc`. PR-URL: #26265 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@BridgeARBridgeAR mentioned this pull request Mar 4, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs 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++.reportIssues and PRs related to process.report.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@richardlau@nodejs-github-bot@Trott@jasnell@addaleax@cjihrig@mhdawson