Skip to content
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

Replace runtime size check with compile time check #1000

Merged
merged 1 commit into from Jun 20, 2022
Merged

Replace runtime size check with compile time check #1000

merged 1 commit into from Jun 20, 2022

Conversation

tcharding
Copy link
Member

Add a macro const_assert that uses some const declaration trickery to trigger a compile time error if a boolean expression is false.

Replace runtime checks using debug_assert_eq! with the newly defined const_asert! macro.

Note

This PR is the first patch from a recently closed PR. Props to @elichai for the macro idea in the review of that PR.

@tcharding tcharding mentioned this pull request May 24, 2022
Closed
TheBlueMatt
TheBlueMatt previously approved these changes May 24, 2022
View reviewed changes
apoelstra
apoelstra previously approved these changes May 24, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 58eef54

@apoelstra
Copy link
Member

I'll wait for another ACK before merging because the last one caused such a stir, but it seems like everyone's happy with this one :).

Kixunil
Kixunil previously approved these changes May 25, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 58eef54

@apoelstra
Copy link
Member

@tcharding your commit message contains an @ sign (and also it looks like you misspelled assert). Can you reword that commit and force-push?

@tcharding tcharding dismissed stale reviews from Kixunil, apoelstra, and TheBlueMatt via e7f390d May 28, 2022 08:23
@tcharding
Copy link
Member Author

I don't see the @ symbol? I fixed the spelling of assert. Cheers.

@apoelstra
Copy link
Member

Ah, the @ sign is actually in the PR description.

apoelstra
apoelstra previously approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK e7f390d

@tcharding
Copy link
Member Author

Oh yes, I don't put mentions in the commit log because it was told to me a few moths ago, but I never understood why that was a bad thing, if you have a second can you tell me why its a bad idea please?

@apoelstra
Copy link
Member

@tcharding sure -- because merging this commit counts as an @ mention, which will show up in the target's Github notification at the time that the commit is merged, and then again whenever any forks (e.g. rust-elements might eventually become a fork of this lib) pull it in, forever.

@tcharding
Copy link
Member Author

Oh, that would be annoying - cheers.

Kixunil
Kixunil previously approved these changes Jun 16, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK e7f390d

@Kixunil Kixunil added code quality Makes code easier to understand and less likely to lead to problems trivial Obvious, easy and quick to review (few lines or doc-only...) labels Jun 16, 2022
@tcharding
Replace runtime size check with compile time check
7a3bb7d 
Add a macro `const_assert` that uses some const declaration trickery to
trigger a compile time error if a boolean expression is false.

Replace runtime checks using `debug_assert_eq!` with the newly defined
`const_assert!` macro.
@tcharding tcharding dismissed stale reviews from Kixunil and apoelstra via 7a3bb7d June 20, 2022 04:40
@tcharding
Copy link
Member Author

Force push was just a rebase and fix merge conflict in internal_macros.rs. No other changes.

Kixunil
Kixunil approved these changes Jun 20, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 7a3bb7d

@Kixunil Kixunil added the one ack PRs that have one ACK, so one more can progress them label Jun 20, 2022
apoelstra
apoelstra approved these changes Jun 20, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 7a3bb7d

@apoelstra apoelstra merged commit 18033f7 into rust-bitcoin:master Jun 20, 2022
@tcharding tcharding deleted the 05-25-const-assert branch June 23, 2022 03:59
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
@apoelstra
Merge rust-bitcoin/rust-bitcoin#1000: Replace runtime size check with…
6fbe648 
… compile time check

7a3bb7d Replace runtime size check with compile time check (Tobin C. Harding)

Pull request description:

  Add a macro `const_assert` that uses some const declaration trickery to trigger a compile time error if a boolean expression is false.

  Replace runtime checks using `debug_assert_eq!` with the newly defined `const_asert!` macro.

  ## Note

  This PR is the first patch from a [recently closed PR](rust-bitcoin/rust-bitcoin#953). Props to @elichai for the macro idea in the review of that PR.

ACKs for top commit:
  Kixunil:
    ACK 7a3bb7d
  apoelstra:
    ACK 7a3bb7d

Tree-SHA512: cfd4dcf6c66e06796cab6dc49445f0f8c5d4e686893a17735420dccedd75ad7c632d240a5ab92ee47ce459b799daeaf3fdf9c6b77c1b81b09e87197a9f86c5ba
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

@TheBlueMatt TheBlueMatt TheBlueMatt left review comments

Assignees
No one assigned
Labels
code quality Makes code easier to understand and less likely to lead to problems one ack PRs that have one ACK, so one more can progress them trivial Obvious, easy and quick to review (few lines or doc-only...)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tcharding @apoelstra @TheBlueMatt @Kixunil