Declare iterators using auto #15762

pull negatratoron wants to merge 1 commits into bitcoin:master from negatratoron:use-auto changing 31 files +116 −116

negatratoron commented at 10:10 AM on April 6, 2019: none

Just some janitorial work. Also made some slight consistency adjustments such as in crypto_test.cpp.

Edit: apparently can't edit files in the leveldb subtree, also my test system didn't build zeromq so i didn't catch my syntax error.
fanquake added the label Refactoring on Apr 6, 2019
negatratoron force-pushed on Apr 6, 2019
negatratoron force-pushed on Apr 6, 2019
negatratoron force-pushed on Apr 6, 2019
DrahtBot commented at 11:36 AM on April 6, 2019: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #15759 ([p2p] Add 2 outbound blocks-only connections by sdaftuar)
- #15487 ([WIP] descriptor based wallet serialization and import by Sjors)
- #14193 (validation: Add missing mempool locks by MarcoFalke)
- #13686 (ZMQ: Small cleanups in the ZMQ code by domob1812)
- #13057 (refactor pre-selected coin code by instagibbs)
- #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)
- #9384 (CCoinsViewCache code cleanup & deduplication by ryanofsky)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Declare iterators using auto b0dd46a276
promag commented at 1:45 PM on April 8, 2019: member

NACK, I think this makes code less readable. Usually we argue about auto usage in new code, but I don't recall of changing code to use auto.

My preference is to use auto like auto x = new Foo so it's clear the x type.

in src/addrman.cpp:70 in b0dd46a276

  66 | @@ -67,12 +67,12 @@ double CAddrInfo::GetChance(int64_t nNow) const
  67 |  
  68 |  CAddrInfo* CAddrMan::Find(const CNetAddr& addr, int* pnId)
  69 |  {
  70 | -    std::map<CNetAddr, int>::iterator it = mapAddr.find(addr);
  71 | +    auto it = mapAddr.find(addr);

promag commented at 1:46 PM on April 8, 2019:

I'd rater see this changed to std::map<CNetAddr, int>::const_iterator it = ... for instance.

practicalswift commented at 7:10 PM on April 8, 2019:

The following scripted-diff compiles if someone wants to look for ::const_iterator candidates:

git grep -lE '::iterator ' "*.cpp" "*.h" | \
    grep -vE '(crypto_tests.cpp|coins_tests.cpp|limitedmap.h|guiutil.cpp|addrman.cpp|bloom.cpp|net.cpp|net_processing.cpp|coins.cpp|coins.h)' | \
    xargs sed -i 's/::iterator /::const_iterator /g'

jamesob commented at 2:39 PM on April 8, 2019: member

NACK.

From the pull request template:

Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they significantly improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the developer notes, stylistic code changes are usually rejected.

Edit: if you're looking for ways to contribute, I recommend checking out the good first issue tag.
negatratoron commented at 7:54 PM on April 8, 2019: none

CI builds for this patch took 2X-3X longer than builds for contemporary patches, and one of them failed due to timeout (I don't know how that CI build succeeded thereafter).
promag commented at 11:11 PM on April 8, 2019: member

@jeffersoncarpenter CI time varies and depends on current build cache. Sometimes failure builds are restarted manually by people with the required permissions.

However, after the above comments, I think you should close this and follow @jamesob advice or @practicalswift #15762 (review) suggestion.
negatratoron commented at 5:03 AM on April 9, 2019: none

Closing.
negatratoron closed this on Apr 9, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Declare iterators using auto #15762

Conflicts