-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Conversation
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.
Looks really good! Just a few small things on my side
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.
The API itself is clean, but I'm generally concerned with defaulting to zero values as None
for integers.
I wonder if instead we should architect this type around "nonzero" integers, so the distinction is clear?
https://docs.rs/bytemuck/latest/bytemuck/trait.ZeroableInOption.html
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.
This is on the right track, @febo! I've just added some suggestions on the trait implementations.
I still think you can add to this type's API to make its use a little bit more ergonomic. I still believe it should at least offer is_some()
and is_none()
directly on the type.
Up to you, but I think COption
does a great job of providing an API similar to Rust's Option
, which makes it feel more comfortable to use.
https://github.com/anza-xyz/agave/blob/master/sdk/program/src/program_option.rs
@buffalojoec I cleaned up the API based on your review, let me know what you think. |
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.
Looks great! Ship it!
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.
Just a few flyby comments that I hope will remove some footguns
fn from(from: Option<T>) -> Self { | ||
match from { | ||
Some(value) => PodOption(value), | ||
None => PodOption(T::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.
This assumes that T::default()
is the None
value, which is not guaranteed by the trait. I understand the desire to make the value flexible, so to make that behavior less error-prone, how about changing the trait to something like:
pub trait Nullable: Pod + PartialEq + Sized {
/// Value indicating the `None` to check against
const NONE: Self;
/// Indicates whether the value is `None` or not.
fn is_none(&self) -> bool {
self == &Self::NONE
}
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.
The only caveat is that in this case NONE
value has to be a const
. I don't think is a big deal, but perhaps we can consider adding a method to the trait to specify the NONE
value.
- Using
const NONE
:impl Nullable for Pubkey { const NONE: Self = Pubkey::new_from_array([0u8; PUBKEY_BYTES]); }
- Using
none()
method:impl Nullable for Pubkey { fn none() -> Self { Pubkey::default() } }
#### Problem The publish step is great, but there's still some manual work to add the changelog and correct title for the created release. #### Summary of changes Use git-cliff to generate the release notes. This comes with a very simple cliff.toml which will do minimal processing, since we don't follow conventional commits in the repo. Here's a test run for the given input: ``` $ git-cliff -c ci/cliff.toml "pod-v0.3.0"..master --include-path "libraries/pod/**" --github-repo "solana-labs/solana-program-library" ``` Output: ``` ## What's new - Publish pod v0.3.2 by @github-actions[bot] - [token-2022] Upgrade to `zk-sdk` (solana-labs#7148) by @samkim-crypto - Implement `Default` for `PodOption` (solana-labs#7083) by @joncinque - Bump to 0.3.1 (solana-labs#7075) by @febo - Improve `PodOption` type (solana-labs#7076) by @febo - Add `PodU128` type (solana-labs#7012) by @febo - Add `PodOption` type (solana-labs#6886) by @febo - Use `bytemuck_derive` explicitly (solana-labs#6928) by @joncinque ```
* CI: Add git-cliff step during publish #### Problem The publish step is great, but there's still some manual work to add the changelog and correct title for the created release. #### Summary of changes Use git-cliff to generate the release notes. This comes with a very simple cliff.toml which will do minimal processing, since we don't follow conventional commits in the repo. Here's a test run for the given input: ``` $ git-cliff -c ci/cliff.toml "pod-v0.3.0"..master --include-path "libraries/pod/**" --github-repo "solana-labs/solana-program-library" ``` Output: ``` ## What's new - Publish pod v0.3.2 by @github-actions[bot] - [token-2022] Upgrade to `zk-sdk` (#7148) by @samkim-crypto - Implement `Default` for `PodOption` (#7083) by @joncinque - Bump to 0.3.1 (#7075) by @febo - Improve `PodOption` type (#7076) by @febo - Add `PodU128` type (#7012) by @febo - Add `PodOption` type (#6886) by @febo - Use `bytemuck_derive` explicitly (#6928) by @joncinque ``` * Don't always run the step * Make the YAML valid * Trim each commit line to just the first line
Problem
Optional values usually require an extra
byte
to indicate whether the value is present or not. This can affect the alignment of a type, which is not ideal.Solution
This PR adds a
PodOption
, which is a generic type that can be used as aPod
to wrap a type as aOption
. It works over types that implement aNullable
trait in order to indicate whether its value represent aNone
or not.For example, a
PodOption<u8>
assumes the a0
value is aNone
and any value different than0
isSome
.The PR includes implementation for all integer types and
Pubkey
.