Remove redundancies: redundant forward declaration, redundant namespace, redundant copying, redundant conditionals #14506

• Remove redundant forward declaration
• Remove confusing variable shadowing
• Remove empty namespace
• Remove unnecessary copying: pass large object by const reference instead
• Remove redundant condition: nBytes < 0 is always true
• Remove redundant condition: iter < 2 is always true

Rationale:

• Less is more

NAK. Too much work to review. In a random spotcheck, I looked at the commit removing the nbytes <0 case, and nBytes is an int. AFAICT the removal just made the code more brittle, because the assumption that it could never be true doesn't flow directly from the structure, but only from the specific behaviour of the surrounding code.

Looks like these changes come more often than what maintainers can take, as such it reduces their available time on other type of changes. Not saying this isn't important, but piling up PR's also doesn't help.

@gmaxwell I see your argument regarding a scenario where the rest of the if-else-flow is changed in a sloppy way: I've now removed the two commits pertaining to redundant conditions. Hopefully the rest of the remaining changes are trivial to review.

From what I can judge they are all technically in the "if it compiles it works" camp. Let me know if that is technically incorrect. (I'm talking in technical terms here – obviously we shouldn't blindly trust the compiler.)

@promag That's why I've batched them into one PR :-)

Currently you have 34 open PR's (more than 10% of the total), 17 of which have Refactoring label — and not counting commits. I'm just saying you can avoid having so many at once and keep the most important open. My opinion of course.

@promag Please note that the "refactoring" labelled PR:s include undefined behaviour fixes and locking fixes :-D

@practicalswift Please don't mix refactoring changes and actual fixes of issues.

A certain degree of refactoring improvements is fine, but you're single-handedly burying reviewers under piles of pull requests. Do maybe one at a time...

Otherwise, PRs that actually fix issues may not get the attention they need because reviewers just see "oh yet another practicalswift PR"...

Concerning actual issues, also please assess what the impact is; many code smell problems while possibly errorneous don't actually have any code path to trigger them.

@sipa I've never called them "refactoring". I call them "undefined behaviour" or simply "bugs" :-)

See my attempt at discussing the issue in https://github.com/bitcoin/bitcoin/pull/14239

@practicalswift I updated my comment above a bit.

"oh yet another practicalswift PR"

Unfortunately that's the feeling. For instance, I could open a lot of PR's to improve/refactor the UI but it wouldn't work for others.

BTW, it would be great to speak to you in IRC instead.

@promag Please get in touch on Twitter and we can speak: https://twitter.com/practicalswift :-)

Also not the best place to have these discussions?

@promag I'm reachable via e-mail too. The username you'd expect at the default mail provider :-)

SHA-1 of e-mail address: 299f0d87208b28e15278cb4976fd72159fe4c3fb

Talk to you soon!

Follow-up:

I've done some cleaning! @promag I'm now down to 23 open pull requests. FWIW that is only two more PR:s than you have open .-)

My 23 pull requests fall in the following five categories:

1. Fix undefined behaviour: 3
2. Locking work to prevent a.) undefined behaviour in the form of data races (data race is UB) and b.) data inconsistency: 5
3. Document and/or fix unintentional unsigned integer wraparounds: 4
4. Document assumptions (noexcept/[[nodiscard]]/[[maybe_unused]]/explicit): 5
5. Improve testing: 6

See https://github.com/bitcoin/bitcoin/pulls/practicalswift @promag @sipa

Please let me know if you think any of these pull requests should be closed and I'll do so :-)

It should be added that most of these are very small in terms of net LOC introduced, which hopefully make them relatively easy to review. For me personally at least review work is largely a function of LOC.