Skip to content

Conversation

@jimpo
Copy link
Contributor

@jimpo jimpo commented Sep 8, 2018

These commits have been split out of #14121 because they are fairly independent and that PR is very large.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 8, 2018

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

Conflicts

No conflicts as of last run.

@jimpo jimpo force-pushed the blockfilter-refactor branch from a3d9b90 to 22fd588 Compare September 10, 2018 18:43
@jimpo jimpo force-pushed the blockfilter-refactor branch from 22fd588 to c306209 Compare November 6, 2018 17:13
Copy link
Contributor

@jamesob jamesob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK c306209

Uncontroversial, cute refactoring that only touches yet-unused BIP157&8 block filter machinery.


/** Constructs an empty filter. */
GCSFilter(uint64_t siphash_k0 = 0, uint64_t siphash_k1 = 0, uint8_t P = 0, uint32_t M = 0);
explicit GCSFilter(const Params& params = Params());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confused about why this is marked explicit...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Developer notes: "By default, declare single-argument constructors explicit"

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK c306209

Looks mostly like refactoring with the following changes:

  • m_encoded is initialized with one char 0 instead of an empty vector when constructing an empty filter.
  • The default value of M changed from 0 to 1.

params.m_M = BASIC_FILTER_M;
break;

default:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could remove the default case (which is a failure) to make it a compile time warning instead of a run time exception

Suggested change
default:
// no default case, so the compiler can warn about missing cases

Example:

blockfilter.cpp:247:13: warning: enumeration value 'BASIC' not handled in switch [-Wswitch]
    switch (m_filter_type) {
            ^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, cool. Didn't realize C++ compilers did that analysis on switch statements.

Now the compiler will warn if not all enums are handled in the
switch.
@maflcko maflcko merged commit e4ed8ce into bitcoin:master Dec 22, 2018
maflcko pushed a commit that referenced this pull request Dec 22, 2018
e4ed8ce blockfilter: Remove default clause in switch statement. (Jim Posen)
c306209 blockfilter: Additional constructors for BlockFilter. (Jim Posen)
20b8129 blockfilter: Refactor GCS params into struct. (Jim Posen)

Pull request description:

  These commits have been split out of #14121 because they are fairly independent and that PR is very large.

Tree-SHA512: b9643b159e114df50a295f433e807afe6082db55a2a3a17401c1509b850c71bf5011ab3638863b46663709726be4445be6fde1dec514aec7696135497a9f0183
@jimpo jimpo deleted the blockfilter-refactor branch December 23, 2018 05:14
kwvg added a commit to kwvg/dash that referenced this pull request Aug 2, 2021
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Aug 11, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 12, 2021
UdjinM6 added a commit to dashpay/dash that referenced this pull request Aug 13, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants