-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
preliminaries for bumping nightly to 2023-08-25 #33047
Conversation
3d23d4c
to
ae52c09
Compare
@@ -192,7 +192,7 @@ impl PartialEq for HeaviestSubtreeForkChoice { | |||
impl PartialOrd for HeaviestSubtreeForkChoice { | |||
// Sort by root | |||
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { | |||
self.tree_root.partial_cmp(&other.tree_root) | |||
Some(self.tree_root.cmp(&other.tree_root)) |
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.
cc/ @AshwinSekar
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.
Considering, Ord
is implemented for HeaviestSubtreeForkChoice
, this can also be
Some(self.tree_root.cmp(&other.tree_root)) | |
Some(self.cmp(other)) |
It is actually what the standard library documentation suggests: https://doc.rust-lang.org/nightly/std/cmp/trait.PartialOrd.html#how-can-i-implement-partialord
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.
but comparing the other fields would change semantics, which i'm not trying to do here
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.
PartialOrd
and Ord
implementations must be consistent.
It is part of the trait description:
If Ord is also implemented for Self and Rhs, it must also be consistent with partial_cmp (see the documentation of that trait for the exact requirements).
From https://doc.rust-lang.org/nightly/std/cmp/trait.PartialOrd.html
So, it is actually better to define one in terms of another. Reducing the chance they diverge.
Currently, they are consistent:
solana/core/src/consensus/heaviest_subtree_fork_choice.rs
Lines 201 to 206 in 166f9e2
#[cfg(test)] | |
impl Ord for HeaviestSubtreeForkChoice { | |
fn cmp(&self, other: &Self) -> std::cmp::Ordering { | |
self.tree_root.cmp(&other.tree_root) | |
} | |
} |
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.
yeah that's an ashwin question. don't want to break consensus for the sake of following someone else's rules 😉
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.
Either approach is fine, they both only compare tree roots. Furthermore this impl is only used for tests, we never compare HeaviestSubtreeForkChoice in consensus.
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.
Clippy is really good at finding all these redundancies and unnecessary complications!
@@ -192,7 +192,7 @@ impl PartialEq for HeaviestSubtreeForkChoice { | |||
impl PartialOrd for HeaviestSubtreeForkChoice { | |||
// Sort by root | |||
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { | |||
self.tree_root.partial_cmp(&other.tree_root) | |||
Some(self.tree_root.cmp(&other.tree_root)) |
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.
Considering, Ord
is implemented for HeaviestSubtreeForkChoice
, this can also be
Some(self.tree_root.cmp(&other.tree_root)) | |
Some(self.cmp(other)) |
It is actually what the standard library documentation suggests: https://doc.rust-lang.org/nightly/std/cmp/trait.PartialOrd.html#how-can-i-implement-partialord
can't wait to not have to nag about some of these thing in review! 😅 |
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 good, i checked the following:
immutable_deseriaized_packet.rs
unprocessed_transaction_storage.rs
cluster_info_vote_listener.rs
commitment_service.rs
consensus.rs
heaviest_subtree_fork_choice.rs
latest_validator_votes_for_frozen_banks.rs
optimistic_confirmation_verifier.rs
cluster_slot_state_verifier.rs
replay_stage.rs
verified_vote_packets.rs
@@ -192,7 +192,7 @@ impl PartialEq for HeaviestSubtreeForkChoice { | |||
impl PartialOrd for HeaviestSubtreeForkChoice { | |||
// Sort by root | |||
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { | |||
self.tree_root.partial_cmp(&other.tree_root) | |||
Some(self.tree_root.cmp(&other.tree_root)) |
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.
Either approach is fine, they both only compare tree roots. Furthermore this impl is only used for tests, we never compare HeaviestSubtreeForkChoice in consensus.
Problem
four months worth of nightly lints
Summary of Changes
fix them shits