Minor improvements to checkpoints code #11339

pull danra wants to merge 1 commits into bitcoin:master from danra:refactor/checkpoints changing 2 files +11 −21

danra commented at 10:15 AM on September 15, 2017: contributor
- Use auto where appropriate, improved var names keep code obvious
- Removed redundant checkpoints intermediate variable in GetLastCheckpoint()
- Remove redundant includes (made more obviously redundant by using auto)
- Remove redundant parameter name in GetLastCheckpoint() declaration
- Whitespace, braces follow developer notes

Minor improvements to checkpoints code

- Use auto where appropriate, improved var names keep code obvious
- Removed redundant 'checkpoints' intermediate variable in GetLastCheckpoint()
- Remove redundant includes (made more obviously redundant by using auto)
- Remove redundant parameter name in GetLastCheckpoint() declaration
- Whitespace, braces follow developer notes

06125d44ee

meshcollider commented at 10:23 AM on September 15, 2017: contributor

NACK tbh, I don't think this improves the code at all, its clearer how it is (especially changes like removing the variable name from the header declaration, where's the advantage in that?)
danra commented at 10:47 AM on September 15, 2017: contributor

@MeshCollider The advantage is just conciseness. What's the advantage of the generic parameter name 'data' for a parameter of type CCheckpointData&?

A parameter name only makes sense in a function declaration if it helps understand the semantics, or if it is referenced somewhere (e.g. documentation)
laanwj added the label Refactoring on Sep 15, 2017
promag commented at 1:08 PM on September 15, 2017: member

Agree with @MeshCollider. IMO if this is merged then it opens the door to a lot of similar PR's.
MarcoFalke commented at 1:19 PM on September 15, 2017: member

At the moment we don't have the resources to review formatting only pulls. Going to close for now.
MarcoFalke closed this on Sep 15, 2017
jnewbery commented at 1:27 PM on September 15, 2017: member

Thanks for this @danra . I think some of the changes in here are good (removing the redundant includes and intermediate parameters), but as others have pointed out, the review/test overhead for small formatting PRs like this tend to outweigh the benefit.
danra commented at 3:23 PM on September 15, 2017: contributor

@MarcoFalke @jnewbery Too bad. This isn't only formatting, and making the code clearer and more concise is important for its security and worth the investment IMHO.
jnewbery commented at 4:59 PM on September 15, 2017: member

@danra - I wouldn't frame this as worth the investment or not. Any change to the consensus code is a risk. There's a judgement on whether that's a good trade-off. In this case my view is no, but that's not to say other people would agree with me.

Plus, using auto in consensus code seems to be strongly not preferred.
DrahtBot locked this on Sep 8, 2021

Contributors

Labels