report: misc cleanup II #25610

addaleax · Jan 21, 2019

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


      report: remove `internalBinding('config').hasReport`

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


      report: roll extra loop iteration in `PrintNativeStack()`


      report: downgrade reinterpret_cast to static_cast


      report: use `uv_handle_type_name()` to get handle type

nodejs-github-bot · Jan 21, 2019

@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

cjihrig · 2019-01-21T15:06:03Z

cjihrig approved these changes

Jan 21, 2019

View changes

src/node_report_utils.cc


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

addaleax · Jan 21, 2019

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

mscdex · Jan 21, 2019

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

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

addaleax · Jan 24, 2019

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

richardlau · 2019-01-24T19:07:30Z

richardlau approved these changes

Jan 24, 2019

View changes

richardlau · Jan 24, 2019

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

addaleax · Jan 24, 2019

Landed in f4697ba...5860614


      report: remove `internalBinding('config').hasReport`

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>


      src: remove unnecessary `filename` variable

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


      report: roll extra loop iteration in `PrintNativeStack()`

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


      report: downgrade reinterpret_cast to static_cast

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


      report: use `uv_handle_type_name()` to get handle type

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

MylesBorins · Jan 24, 2019

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.


      report: remove `internalBinding('config').hasReport`

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>


      src: remove unnecessary `filename` variable

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


      report: roll extra loop iteration in `PrintNativeStack()`

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


      report: downgrade reinterpret_cast to static_cast

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


      report: use `uv_handle_type_name()` to get handle type

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

addaleax added 5 commits Jan 21, 2019

src: remove unnecessary `filename` variable

Verified

This commit was signed with a verified signature.

addaleax Anna Henningsen

GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits

768c1fd

report: roll extra loop iteration in `PrintNativeStack()`

Verified

This commit was signed with a verified signature.

addaleax Anna Henningsen

GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits

7e0bed1

report: downgrade reinterpret_cast to static_cast

Verified

This commit was signed with a verified signature.

addaleax Anna Henningsen

GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits

3130406

report: use `uv_handle_type_name()` to get handle type

Verified

This commit was signed with a verified signature.

addaleax Anna Henningsen

GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits

Loading status checks…

ec5ee3e

nodejs-github-bot added C++ lib / src labels Jan 21, 2019

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

Verified

This commit was signed with a verified signature.

addaleax Anna Henningsen

GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits

Loading status checks…

d356f5b

addaleax requested a review from gireeshpunathil Jan 22, 2019

addaleax added the report label Jan 22, 2019

addaleax closed this Jan 24, 2019

addaleax deleted the addaleax:report-cleanup branch Jan 24, 2019

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

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

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

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

MylesBorins referenced this pull request Jan 24, 2019

v11.8.0 proposal #25687

Merged

nodejs/node

report: misc cleanup II #25610

report: misc cleanup II #25610

addaleax commented Jan 21, 2019

This comment has been minimized.

nodejs-github-bot commented Jan 21, 2019

nodejs-github-bot added C++ lib / src labels Jan 21, 2019

This comment has been minimized.

addaleax commented Jan 21, 2019 •

edited

This comment has been minimized.

mscdex commented Jan 21, 2019

This comment has been minimized.

addaleax commented Jan 21, 2019

addaleax requested a review from gireeshpunathil Jan 22, 2019

addaleax added the report label Jan 22, 2019

This comment has been minimized.

addaleax commented Jan 24, 2019

This comment has been minimized.

richardlau commented Jan 24, 2019

This comment has been minimized.

addaleax commented Jan 24, 2019

addaleax closed this Jan 24, 2019

addaleax deleted the addaleax:report-cleanup branch Jan 24, 2019

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

This comment has been minimized.

MylesBorins commented Jan 24, 2019

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

nodejs/node

Join GitHub today

report: misc cleanup II #25610

Conversation

addaleax commented Jan 21, 2019

Checklist

This comment has been minimized.

nodejs-github-bot commented Jan 21, 2019

nodejs-github-bot added C++ lib / src labels Jan 21, 2019

This comment has been minimized.

addaleax commented Jan 21, 2019 • edited

This comment has been minimized.

mscdex commented Jan 21, 2019

This comment has been minimized.

addaleax commented Jan 21, 2019

addaleax requested a review from gireeshpunathil Jan 22, 2019

addaleax added the report label Jan 22, 2019

This comment has been minimized.

addaleax commented Jan 24, 2019

This comment has been minimized.

richardlau commented Jan 24, 2019

This comment has been minimized.

addaleax commented Jan 24, 2019

addaleax closed this Jan 24, 2019

addaleax deleted the addaleax:report-cleanup branch Jan 24, 2019

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

This comment has been minimized.

MylesBorins commented Jan 24, 2019

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

addaleax commented Jan 21, 2019 •

edited