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

report: misc cleanup II #25610

Closed
wants to merge 6 commits into from

Conversation

@addaleax
Copy link
Member

commented 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
addaleax added 5 commits Jan 21, 2019
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/node_report_utils.cc Outdated Show resolved Hide resolved
@addaleax

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

@mscdex

This comment has been minimized.

Copy link
Contributor

commented 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

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

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

@addaleax addaleax requested a review from gireeshpunathil Jan 22, 2019

@addaleax addaleax added the report label Jan 22, 2019

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2019

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

@richardlau

This comment has been minimized.

Copy link
Member

commented 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

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2019

Landed in f4697ba...5860614

@addaleax addaleax closed this Jan 24, 2019

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

addaleax added a commit that referenced this pull request Jan 24, 2019
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>
addaleax added a commit that referenced this pull request Jan 24, 2019
src: remove unnecessary `filename` variable
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()`
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
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
PR-URL: #25610
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

commented 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.

addaleax added a commit that referenced this pull request Jan 24, 2019
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>
addaleax added a commit that referenced this pull request Jan 24, 2019
src: remove unnecessary `filename` variable
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()`
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
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
PR-URL: #25610
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.