Skip to content

report: fix build warning in node_report.cc - #26265

Merged
richardlau merged 1 commit into
nodejs:masterfrom
richardlau:uninitializedoutstream
Feb 28, 2019
Merged

report: fix build warning in node_report.cc#26265
richardlau merged 1 commit into
nodejs:masterfrom
richardlau:uninitializedoutstream

Conversation

@richardlau

@richardlaurichardlau commented Feb 22, 2019

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Comment threadsrc/node_report.cc Outdated
@richardlau

Copy link
Copy Markdown
MemberAuthor

CI will need to wait until the lockdown ends.

@Trott

Copy link
Copy Markdown
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
Copy Markdown
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

Trott commented Feb 22, 2019

Copy link
Copy Markdown
Member

@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
Copy Markdown
Member

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

@richardlau
richardlauforce-pushed the uninitializedoutstream branch from 1fc7bb9 to ec6420cCompareFebruary 22, 2019 23:40
@richardlau

Copy link
Copy Markdown
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
Copy Markdown
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....

@mhdawsonmhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@Trott

Copy link
Copy Markdown
Member

@richardlau

richardlau commented Feb 28, 2019

Copy link
Copy Markdown
MemberAuthor

@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 <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@richardlau
richardlauforce-pushed the uninitializedoutstream branch from ec6420c to 4a10ce6CompareFebruary 28, 2019 16:32
@richardlau
richardlau merged commit 4a10ce6 into nodejs:masterFeb 28, 2019
@richardlau

Copy link
Copy Markdown
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 <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@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