Improve const_assert by tcharding · Pull Request #3215 · rust-bitcoin/rust-bitcoin

tcharding · 2024-08-22T07:43:50Z

Now that we can panic in const context we can improve the const_assert macro by adding a message string.

Original idea by Kix:

Kixunil

Other than + vs ? this is good.

Kixunil · 2024-08-22T07:46:43Z

internals/src/macros.rs

This needs to be ?, not +. If you somehow want to support multiple messages then this syntax would look better: $(; $(mesage:expr),+ $(,)?)?

Drats, that was just a mistake - I'm sick and brain fried right now, you might have noticed. I'm enjoying coding though. Please excuse me.

Kixunil · 2024-08-22T07:48:58Z

internals/src/lib.rs

Actually, this message should say "platforms that have usize larger than 64 bits are not supported" so that it's clear it's a compatibility problem for it to fail.

Used as suggested, thanks.

Kixunil · 2024-08-23T05:30:45Z

internals/src/macros.rs

Oh, actually, this has no separator for message so it'd become "failedplatforms". I think that rather than adding it to the string above we should put additional ": ", after $(,.

Done, and tested with

#[cfg(test)] mod tests { use super::*; #[test] fn const_assert() { const_assert!(1 == 0; "this is the message"); } }

Which outputs:

---- macros::tests::const_assert stdout ---- thread 'macros::tests::const_assert' panicked at internals/src/macros.rs:212:9: assertion 1 == 0 failed: this is the message

This made me realize our macro is misleading. It's not actually a const assert unless you assign it into a const. It needs to expand to const _: () = if ...

…rtibility of `u32` to `usize` c427d8b bitcoin: Compile time assert on index size (Tobin C. Harding) 49a6acc internals: Remove double parenthesis in const_assert (Tobin C. Harding) 2300b28 units: Remove compile time pointer width check (Tobin C. Harding) Pull request description: 3 patches in preparation for other size related work, this PR does not touch the `ToU64` issue which will be handled separately. - Patch 1: Don't check pointer width in `units` because its not consensus code - Patch 2: Modify internal macro `const_assert` - Patch 3: Use index size to enforce not building on a 16 bit machine ACKs for top commit: Kixunil: ACK c427d8b though I think the last commit was kinda a waste of time and it should have been adding the trait instead or leave it for later. apoelstra: ACK c427d8b successfully ran local tests; unsure if we want to merg this or wait for #3215 Tree-SHA512: 823df5b6a5af3265bce2422c00d287f45816faeb5f965685650ac974a1bd441cf548e25ac2962591732ff221bee91a55703da936382eb166c014ca5d4129edf8

tcharding · 2024-08-29T01:00:44Z

Now adds a module so we can use const_asset in item context. (I couldn't get double parenthesis to work.)

Kixunil · 2024-08-29T07:14:56Z

internals/src/macros.rs

This is broken because previously it expanded to const _, now it doesn't.

Oh, nice. Now we can get rid of the module again.

Now that we can panic in const context we can improve the `const_assert` macro by adding a message string. Original idea by Kix: rust-bitcoin#2972 (comment)

Kixunil

ACK 3c7c8c4 in the sense that it does what it's supposed to with the only tiny issue being that the bool looks weird but not broken in any other way I can think of.

Kixunil · 2024-08-30T15:59:18Z

internals/src/macros.rs

-        const _: [(); 0 - !$x as usize] = [];
-    };
+    ($x:expr $(; $message:expr)?) => {
+        const _: bool = {


Why? It could've been just ().

Good to call this out so that if we later dig into this, we at least can see that we noticed it and thought it was funny at the time.

But it really makes no difference at all. I suspect it could have also been ! if you want the simplest possible type.

It'd have to always panic to be !.

Seemed logical to me that if one is asserting an expression then the expression is a bool. I agree with the observation that it is not technically required.

FTR none of the asserts in std evaluates to bool for a simple reason: the return value would be always true. Same goes here except you can't even compile a const that's not true, that's why it carries no information which is better represented by () type.

apoelstra

ACK 3c7c8c4 successfully ran local tests

It was correctly pointed out during review of rust-bitcoin#3215 (when we made `const_assert` panic) that using a `bool` added no additional information. Remove the `bool` and just use unit.

c71b23d Remove bool from cont_assert (Tobin C. Harding) Pull request description: It was correctly pointed out during review of #3215 (when we made `const_assert` panic) that using a `bool` added no additional information. Remove the `bool` and just use unit. ACKs for top commit: apoelstra: ACK c71b23d successfully ran local tests; lol sure Kixunil: ACK c71b23d Tree-SHA512: be9f4f10ee7d626a082b7ae9f257b79d500824ed3c1f7327391b2ad4d67e60d7da47a14fa7ef8f99d1ea8157967b4658518cbcf1c1bfcf1d8888455f3eb96437

github-actions bot added the C-internals PRs modifying the internals crate label Aug 22, 2024

Kixunil reviewed Aug 22, 2024

View reviewed changes

apoelstra mentioned this pull request Aug 22, 2024

Use index size rather than pointer size to enforce convertibility of u32 to usize #2972

Merged

tcharding force-pushed the 08-22-const-assert branch 2 times, most recently from 356aa0d to 8c74534 Compare August 23, 2024 04:50

Kixunil requested changes Aug 23, 2024

View reviewed changes

tcharding force-pushed the 08-22-const-assert branch from 8c74534 to d5eb992 Compare August 29, 2024 00:59

github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Aug 29, 2024

tcharding force-pushed the 08-22-const-assert branch from d5eb992 to b4abb23 Compare August 29, 2024 01:00

Kixunil requested changes Aug 29, 2024

View reviewed changes

Improve const_assert

3c7c8c4

Now that we can panic in const context we can improve the `const_assert` macro by adding a message string. Original idea by Kix: rust-bitcoin#2972 (comment)

tcharding force-pushed the 08-22-const-assert branch from b4abb23 to 3c7c8c4 Compare August 29, 2024 20:33

tcharding requested a review from Kixunil August 29, 2024 20:34

Kixunil approved these changes Aug 30, 2024

View reviewed changes

apoelstra approved these changes Aug 31, 2024

View reviewed changes

apoelstra merged commit 753961f into rust-bitcoin:master Aug 31, 2024

tcharding mentioned this pull request Sep 1, 2024

Remove bool from const_assert #3280

Merged

tcharding deleted the 08-22-const-assert branch September 5, 2024 22:13

Conversation

tcharding commented Aug 22, 2024

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Aug 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tcharding Aug 22, 2024 •

edited

Loading