Skip to content

[Draft] Added const power of two check for capacity#62

Closed
zimward wants to merge 0 commit intoNULLx76:masterfrom
zimward:master
Closed

[Draft] Added const power of two check for capacity#62
zimward wants to merge 0 commit intoNULLx76:masterfrom
zimward:master

Conversation

@zimward
Copy link

@zimward zimward commented Aug 18, 2022

Implements #61 causing a compile error when trying to construct a ConstGenericRingBuffer with a capacity of zero or non power of two value.

This is only a draft and requires generic_const_exprs and should not be merged.

Comment on lines +270 to +275
pub struct Assert<const COND: bool> {}
pub trait IsTrue {}
impl IsTrue for Assert<true> {}
pub const fn is_power_of_two(n: usize) -> bool {
n != 0 && n & (n - 1) == 0
}
Copy link

@danielSanchezQ danielSanchezQ Sep 12, 2022

Choose a reason for hiding this comment

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

This is smart. Although it feels something forced to be used here.
Not sure if it can be done but imo, what would be better would be to make sure constructors are const so const expressions can be used. Use a const expr to assert the size in the constructors. That way you do not need to spread that where Assert<...>: IsTrue everywhere.
But again, just a curious traveling through. Since I felt the same when I read about this in the documentation and had the same idea.

Copy link

@danielSanchezQ danielSanchezQ Sep 12, 2022

Choose a reason for hiding this comment

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

Actually, just saw it is almost implemented here

Copy link
Author

Choose a reason for hiding this comment

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

To be fair i only saw a simmilar pattern used somewhere and copied it for my self. I didn't test the PR you've linked, but it seems to be what i intended.

@zimward zimward marked this pull request as draft October 23, 2022 20:39
@zimward zimward closed this Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants