[lib] Refactor bloom filter class data member names #16336

pull qmma70 wants to merge 1 commits into bitcoin:master from qmma70:bloom_refactor changing 2 files +78 −78

qmma70 commented at 1:44 AM on July 4, 2019: contributor

Class member variables have a m_ prefix.

Refactor data members of CBloomFilter and CRollingBloomFilter following the above convention to make them more obvious to maintainers.
Refactor bloom filter class data member names d2e6cf51dc
fanquake added the label Refactoring on Jul 4, 2019

in src/bloom.cpp:52 in d2e6cf51dc

  56 |  {
  57 | -    if (isFull)
  58 | +    if (m_isFull)
  59 |          return;
  60 | -    for (unsigned int i = 0; i < nHashFuncs; i++)
  61 | +    for (unsigned int i = 0; i < m_nHashFuncs; ++i)

fqlx commented at 3:44 AM on July 4, 2019:

Unnecessary to increment the counter here.

in src/bloom.cpp:34 in d2e6cf51dc
```
  35 | -    isFull(false),
  36 | -    isEmpty(true),
  37 | -    nHashFuncs(std::min((unsigned int)(vData.size() * 8 / nElements * LN2), MAX_HASH_FUNCS)),
  38 | -    nTweak(nTweakIn),
  39 | -    nFlags(nFlagsIn)
  40 | +    m_isFull(false),
```
fqlx commented at 3:45 AM on July 4, 2019:

The code is follow camelCase and you are changing it to underscore_case. Bad decision. Follow the current style.

qmma70 commented at 3:55 AM on July 4, 2019:

The m_ convention denotes that the variable is a data member of the class. It is an aid for maintainers to avoid creating side effects by mistake. It doesn't contradict with the current style.

fqlx commented at 3:58 AM on July 4, 2019:

I understand! Thanks!

in src/bloom.cpp:264 in d2e6cf51dc

 271 | +            m_data[p] = p1 & mask;
 272 | +            m_data[p + 1] = p2 & mask;
 273 |          }
 274 |      }
 275 | -    nEntriesThisGeneration++;
 276 | +    ++m_nEntriesThisGeneration;

fqlx commented at 3:45 AM on July 4, 2019:

Prefix is unnecessary. Can you explain?

sipa commented at 3:46 AM on July 4, 2019: member

NACK, per developer guidelines, do not submit patches that purely improve style.

in src/bloom.cpp:251 in d2e6cf51dc

 251 | -        if (nGeneration == 4) {
 252 | -            nGeneration = 1;
 253 | +    if (m_nEntriesThisGeneration == m_nEntriesPerGeneration) {
 254 | +        m_nEntriesThisGeneration = 0;
 255 | +        ++m_nGeneration;
 256 | +        if (m_nGeneration == 4) {

fqlx commented at 3:46 AM on July 4, 2019:

4 should be in a enum so we understand the meaning

in src/bloom.cpp:252 in d2e6cf51dc

 252 | -            nGeneration = 1;
 253 | +    if (m_nEntriesThisGeneration == m_nEntriesPerGeneration) {
 254 | +        m_nEntriesThisGeneration = 0;
 255 | +        ++m_nGeneration;
 256 | +        if (m_nGeneration == 4) {
 257 | +            m_nGeneration = 1;

fqlx commented at 3:46 AM on July 4, 2019:

Same with 1. Needs to be a constant in some enum

fqlx changes_requested
fanquake commented at 3:49 AM on July 4, 2019: member

Thanks, however please read through https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring.
fanquake closed this on Jul 4, 2019
qmma70 deleted the branch on Jul 4, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels