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

wallet: Remove unused local variable old_label #14117

Conversation

Projects
None yet
5 participants
@practicalswift
Copy link
Member

commented Aug 31, 2018

Remove unused local variable old_label.

Last use was removed in #13825.

@fanquake fanquake added the Wallet label Aug 31, 2018

@fanquake

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

I’m curious to know why these don’t get picked up during review, instead of always “just after” a PR has been merged? If there is a too being used, can it be run on the PRs?

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2018

@fanquake I do quite a lot of checking using various automated jobs, this includes: custom linting, running static analysers on the code base, running tests under dynamic analysers, spell checking of documentation, compiling under various compilers with a wide range of warning flags enabled, etc, etc. These jobs currently run only against master due to resource limitations.

Ideally these checks would run in Travis (assuming the false positive rate is near zero and the runtime is low): that would make sure these issues don't reach master, solve the resource limitations and keep the entire process automated. Could it be better? :-)

In order to catch a larger proportion of these issues pre-merge please help reaching that goal by taking a few minutes to review some of these open PRs of mine:

  • To allow for automated thread safety checking (-Wthread-safety-analysis) under Travis: please help by reviewing #11634, #11652, #13115, #13123, #13128, #14108 and the tracking issue #13129
  • To allow for automated detection of accidentally ignored return values ([[nodiscard]]) under Travis: please help by reviewing #13864 and #13815
  • To allow for automated integer overflow/wraparound checking (-fsanitize=integer) under Travis: please help by reviewing #11535 and #11551
  • To allow for automated spell checking (codespell) under Travis: please help by reviewing #13954
  • To allow for automated checking of Doxygen comments (-Wdocumentation) under Travis: please help by reviewing #13914
  • To allow for automated detection of general issues – please help increase signal to noise by fixing/avoiding compiler warnings or static analyzer warnings (by fixing the cause of the warning at hand or by simply guiding the static analyzers better): please help by reviewing #14117 (this PR), #14094, #14088, #13969, #13909, #13897, #13548, #13766, #13546, #13392, #13382 and #13249.
  • To improve the linting/fuzzing/testing infrastructure in general: please help by reviewing #14092, #14010 and #14115

Thanks for helping out!

The C++ Core Guidelines sum up my view on the benefits of mechanical checking over human reviewing for this subclass of issues:

Enforcement might be done by code review, by static analysis, by compiler, or by run-time checks. Wherever possible, we prefer “mechanical” checking (humans are slow, inaccurate, and bore easily) and static checking.

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

I’m curious to know why these don’t get picked up during review, instead of always “just after” a PR has been merged? If there is a too being used, can it be run on the PRs?

IMO, it would make more sense to do a periodical PR that cleans up these things, grouped together, instead of "just after" a PR.

There's really no hurry to get rid of an unused local variable, no need to immediately open a PR.

We've discussed this before.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

Agree with @laanwj. If there was a way to make travis yell at you when there is a "unused local variable" or $some_other_non_critical_but_should_be_cleaned_up_before_the_next_major_release_style_issue, then fix them immediately on master if they slip in for whatever reason.

Otherwise a monthly pull request with all minor fixes (or one right before branch-off every six months) should be enough. Note that you can always keep a local master branch with all you fixes, so that your automated jobs are happy during that time.

@promag

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

utCAK 0e5ac7e.

Agree with the batch cleanup.

We could add a GH label specific to "request" @practicalswift analysis so he can filter those?

@fanquake

This comment has been minimized.

Copy link
Member

commented Sep 2, 2018

This can be combined into #14094, as both are removing unused variables.

@fanquake fanquake closed this Sep 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.