report: misc cleanup II by addaleax · Pull Request #25610 · nodejs/node

addaleax · 2019-01-21T14:40:37Z

The first commit is very whitespace-rich, so it’s worth using the ?w=1 variant of the diff.

Checklist

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

The `setup()` method is only called when the `--experimental-report` option is set. `getOptionValue()` returns `undefined` when the flag is not defined, so the extra check inside of `setup()` is redundant.

nodejs-github-bot · 2019-01-21T14:40:39Z

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2317/pipeline

src/node_report_utils.cc

addaleax · 2019-01-21T15:08:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/20231/ (:heavy_check_mark:)

mscdex · 2019-01-21T22:02:14Z

FWIW here is a compiler warning I'm seeing in master if you want to include a fix for it as part of the cleanup:

../src/node_report.cc: In function ‘void report::PrintSystemInformation(report::JSONWriter*)’:
../src/node_report.cc:778:58: warning: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘rlim_t {aka long unsigned int}’ [-Wformat=]
         snprintf(buf, sizeof(buf), "%llu", limit.rlim_max);
                                            ~~~~~~~~~~~~~~^

addaleax · 2019-01-21T22:04:05Z

@mscdex See #25597 (comment) for what I’d like to do/why this is isn’t included here :)

addaleax · 2019-01-24T18:48:24Z

Maybe @lundibundi or @richardlau would be interested in reviewing this?

richardlau · 2019-01-24T19:09:22Z

FWIW re. the Travis commit message linting failure -- I've raised nodejs/core-validate-commit#46 to add report as a valid subsystem.

addaleax · 2019-01-24T19:49:02Z

Landed in f4697ba...5860614

The `setup()` method is only called when the `--experimental-report` option is set. `getOptionValue()` returns `undefined` when the flag is not defined, so the extra check inside of `setup()` is redundant. PR-URL: #25610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

PR-URL: #25610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

MylesBorins · 2019-01-24T21:18:38Z

Should this be backported to v11.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

The `setup()` method is only called when the `--experimental-report` option is set. `getOptionValue()` returns `undefined` when the flag is not defined, so the extra check inside of `setup()` is redundant. PR-URL: #25610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

PR-URL: #25610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

addaleax added 5 commits January 21, 2019 15:03

report: remove internalBinding('config').hasReport

da03f0f

The `setup()` method is only called when the `--experimental-report` option is set. `getOptionValue()` returns `undefined` when the flag is not defined, so the extra check inside of `setup()` is redundant.

src: remove unnecessary filename variable

768c1fd

report: roll extra loop iteration in PrintNativeStack()

7e0bed1

report: downgrade reinterpret_cast to static_cast

3130406

report: use uv_handle_type_name() to get handle type

ec5ee3e

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 Jan 21, 2019

cjihrig approved these changes Jan 21, 2019

View reviewed changes

src/node_report_utils.cc Outdated Show resolved Hide resolved

fixup! report: use uv_handle_type_name() to get handle type

d356f5b

addaleax requested a review from gireeshpunathil January 22, 2019 21:57

addaleax added the report Issues and PRs related to process.report. label Jan 22, 2019

richardlau approved these changes Jan 24, 2019

View reviewed changes

addaleax closed this Jan 24, 2019

addaleax deleted the report-cleanup branch January 24, 2019 19:49

addaleax added a commit that referenced this pull request Jan 24, 2019

src: remove unnecessary filename variable

97f6261

PR-URL: #25610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

addaleax added a commit that referenced this pull request Jan 24, 2019

report: roll extra loop iteration in PrintNativeStack()

6545197

PR-URL: #25610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

addaleax added a commit that referenced this pull request Jan 24, 2019

report: downgrade reinterpret_cast to static_cast

69ce113

PR-URL: #25610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

addaleax added a commit that referenced this pull request Jan 24, 2019

report: use uv_handle_type_name() to get handle type

5860614

PR-URL: #25610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

MylesBorins added the backport-requested-v11.x label Jan 24, 2019

addaleax added a commit that referenced this pull request Jan 24, 2019

src: remove unnecessary filename variable

a161a9b

PR-URL: #25610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

addaleax added a commit that referenced this pull request Jan 24, 2019

report: roll extra loop iteration in PrintNativeStack()

07a0dc8

PR-URL: #25610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

addaleax added a commit that referenced this pull request Jan 24, 2019

report: downgrade reinterpret_cast to static_cast

03ba344

PR-URL: #25610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

addaleax added a commit that referenced this pull request Jan 24, 2019

report: use uv_handle_type_name() to get handle type

f3d0591

PR-URL: #25610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

addaleax removed the backport-requested-v11.x label Jan 24, 2019

MylesBorins mentioned this pull request Jan 24, 2019

v11.8.0 proposal #25687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

report: misc cleanup II#25610

report: misc cleanup II#25610
addaleax wants to merge 6 commits intonodejs:masterfrom
addaleax:report-cleanup

addaleax commented Jan 21, 2019

Uh oh!

nodejs-github-bot commented Jan 21, 2019

Uh oh!

Uh oh!

addaleax commented Jan 21, 2019 •

edited

Loading

Uh oh!

mscdex commented Jan 21, 2019

Uh oh!

addaleax commented Jan 21, 2019

Uh oh!

addaleax commented Jan 24, 2019

Uh oh!

richardlau commented Jan 24, 2019

Uh oh!

addaleax commented Jan 24, 2019

Uh oh!

MylesBorins commented Jan 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

addaleax commented Jan 21, 2019

Checklist

Uh oh!

nodejs-github-bot commented Jan 21, 2019

Uh oh!

Uh oh!

addaleax commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Jan 21, 2019

Uh oh!

addaleax commented Jan 21, 2019

Uh oh!

addaleax commented Jan 24, 2019

Uh oh!

richardlau commented Jan 24, 2019

Uh oh!

addaleax commented Jan 24, 2019

Uh oh!

MylesBorins commented Jan 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

addaleax commented Jan 21, 2019 •

edited

Loading