Skip to content

Allow fields to be opt-in rather than opt-out #45

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 13, 2021
Merged

Allow fields to be opt-in rather than opt-out #45

merged 2 commits into from
Sep 13, 2021

Conversation

theuni
Copy link
Contributor

@theuni theuni commented Jul 21, 2021 •
edited
Loading

This addresses the integration concern seen here: https://github.com/theuni/bitcoin/blob/minisketch-split/src/Makefile.minisketch.include#L4

If a field is explicitly enabled and disabled, disable takes precedence.
Defines are intended to work as such:

Default: All fields enabled
-DDISABLE_FIELD_3: All fields except 3 are enabled
-DENABLE_FIELD_3: All fields enabled
-DDISABLE_DEFAULT_FIELDS: Logical error, no fields enabled
-DDISABLE_DEFAULT_FIELDS -DENABLE_FIELD_3: Only field 3 enabled
-DDISABLE_DEFAULT_FIELDS -DENABLE_FIELD_2 -DENABLE_FIELD_3: Fields 2 and 3 are enabled
-DDISABLE_DEFAULT_FIELDS -DENABLE_FIELD_2 -DENABLE_FIELD_3 -DDISABLE_FIELD_3: Only field 2 enabled

@sipa
Copy link
Collaborator

sipa commented Aug 20, 2021

I think it would be cleaner to have a layer (in one of the .h) files which takes the "external" defines (which can be both opt-in or opt-out based), and define separate internal defines which are opt-in only. That would avoid littering the "explicitly enabled or default enabled and not disabled" logic everywhere.

@theuni
Copy link
Contributor Author

theuni commented Aug 23, 2021

Aha, agreed. Will give that a go.

@theuni
Copy link
Contributor Author

theuni commented Aug 23, 2021

Done. The resulting header is big and ugly, but it's self-contained and fairly straightforward I think.

@theuni theuni changed the title POC: Allow fields to be opt-in rather than opt-out Allow fields to be opt-in rather than opt-out Aug 23, 2021
@theuni
Copy link
Contributor Author

theuni commented Aug 23, 2021

CI failures appear to be unrelated.

@sipa
Copy link
Collaborator

sipa commented Aug 23, 2021

utACK faff8a0

@fanquake
Copy link
Member

CI failures appear to be unrelated.

The issue is that brew is trying to use bintray, which no longer exists. Could be solved by either running a brew update step before installing packages (may take some time to run), or using a newer macOS image which would have a more up-to-date version of brew already available.

@sipa
Copy link
Collaborator

sipa commented Aug 25, 2021

@fanquake Feel like PR'ing a fix?

@fanquake
Copy link
Member

fanquake commented Sep 1, 2021

@fanquake Feel like PR'ing a fix?

Done in #48.

jnewbery
jnewbery reviewed Sep 7, 2021
View reviewed changes
@sipa
Copy link
Collaborator

sipa commented Sep 13, 2021

Squash the squashme?

@theuni
Allow field opt-in
812d428 
Building with -DDISABLE_DEFAULT_FIELDS disables all fields, allowing only the
desired ones to be enabled with -DDENABLE_FIELD_FOO.

If a field is explicitly enabled and disabled, disable takes precedence.
@theuni
Add convenience defines for enabling byte sizes
67f5160
@theuni
Copy link
Contributor Author

theuni commented Sep 13, 2021

Done

@sipa
Copy link
Collaborator

sipa commented Sep 13, 2021

utACK 67f5160

@sipa sipa merged commit ea986a8 into bitcoin-core:master Sep 13, 2021
@fanquake fanquake mentioned this pull request Sep 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@jnewbery jnewbery jnewbery left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@theuni @sipa @fanquake @jnewbery