-
Notifications
You must be signed in to change notification settings - Fork 38.8k
Refactor and add tests for BlockFilter construction #14172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
a3d9b90 to
22fd588
Compare
22fd588 to
c306209
Compare
jamesob
left a comment
There was a problem hiding this 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()); |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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"
maflcko
left a comment
There was a problem hiding this 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_encodedis initialized with one char0instead of an empty vector when constructing an empty filter.- The default value of
Mchanged from0to1.
src/blockfilter.cpp
Outdated
| params.m_M = BASIC_FILTER_M; | ||
| break; | ||
|
|
||
| default: |
There was a problem hiding this comment.
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
| 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) {
^
There was a problem hiding this comment.
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.
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
merge bitcoin#15118, bitcoin#14172, bitcoin#15623, bitcoin#14121, partial bitcoin#13743, partial bitcoin#15280: block filters
These commits have been split out of #14121 because they are fairly independent and that PR is very large.