Skip to content

Commit

Permalink
Remove SelfEntry in favor of Option
Browse files Browse the repository at this point in the history 
Clippy warned that `SelfEntry` ends in the same name as the enum.
We can avoid this by modelling this as an `Option` instead which
also has the benefit of using `?` in several places.
  • Loading branch information
@thomaseizinger
thomaseizinger committed Dec 29, 2023
1 parent d161d08 commit 294c197
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 38 deletions.
36 changes: 21 additions & 15 deletions protocols/kad/src/behaviour.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ where
};
let key = kbucket::Key::from(*peer);
match self.kbuckets.entry(&key) {
kbucket::Entry::Present(mut entry, _) => {
Some(kbucket::Entry::Present(mut entry, _)) => {
if entry.value().insert(address) {
self.queued_events
.push_back(ToSwarm::GenerateEvent(Event::RoutingUpdated {
Expand All @@ -536,11 +536,11 @@ where
}
RoutingUpdate::Success
}
kbucket::Entry::Pending(mut entry, _) => {
Some(kbucket::Entry::Pending(mut entry, _)) => {
entry.value().insert(address);
RoutingUpdate::Pending
}
kbucket::Entry::Absent(entry) => {
Some(kbucket::Entry::Absent(entry)) => {
let addresses = Addresses::new(address);
let status = if self.connected_peers.contains(peer) {
NodeStatus::Connected
Expand Down Expand Up @@ -578,7 +578,7 @@ where
}
}
}
kbucket::Entry::SelfEntry => RoutingUpdate::Failed,
None => RoutingUpdate::Failed,
}
}

Expand All @@ -599,7 +599,7 @@ where
) -> Option<kbucket::EntryView<kbucket::Key<PeerId>, Addresses>> {
let address = &address.to_owned().with_p2p(*peer).ok()?;
let key = kbucket::Key::from(*peer);
match self.kbuckets.entry(&key) {
match self.kbuckets.entry(&key)? {
kbucket::Entry::Present(mut entry, _) => {
if entry.value().remove(address).is_err() {
Some(entry.remove()) // it is the last address, thus remove the peer.
Expand All @@ -614,7 +614,7 @@ where
None
}
}
kbucket::Entry::Absent(..) | kbucket::Entry::SelfEntry => None,
kbucket::Entry::Absent(..) => None,
}
}

Expand All @@ -627,10 +627,10 @@ where
peer: &PeerId,
) -> Option<kbucket::EntryView<kbucket::Key<PeerId>, Addresses>> {
let key = kbucket::Key::from(*peer);
match self.kbuckets.entry(&key) {
match self.kbuckets.entry(&key)? {
kbucket::Entry::Present(entry, _) => Some(entry.remove()),
kbucket::Entry::Pending(entry, _) => Some(entry.remove()),
kbucket::Entry::Absent(..) | kbucket::Entry::SelfEntry => None,
kbucket::Entry::Absent(..) => None,
}
}

Expand Down Expand Up @@ -1164,7 +1164,8 @@ where
let key = kbucket::Key::from(node_id);
kbuckets
.entry(&key)
.view()
.as_mut()
.and_then(|e| e.view())
.map(|e| e.node.value.clone().into_vec())
}
} else {
Expand Down Expand Up @@ -1220,7 +1221,7 @@ where
) {
let key = kbucket::Key::from(peer);
match self.kbuckets.entry(&key) {
kbucket::Entry::Present(mut entry, old_status) => {
Some(kbucket::Entry::Present(mut entry, old_status)) => {
if old_status != new_status {
entry.update(new_status)
}
Expand All @@ -1243,7 +1244,7 @@ where
}
}

kbucket::Entry::Pending(mut entry, old_status) => {
Some(kbucket::Entry::Pending(mut entry, old_status)) => {
if let Some(address) = address {
entry.value().insert(address);
}
Expand All @@ -1252,7 +1253,7 @@ where
}
}

kbucket::Entry::Absent(entry) => {
Some(kbucket::Entry::Absent(entry)) => {
// Only connected nodes with a known address are newly inserted.
if new_status != NodeStatus::Connected {
return;
Expand Down Expand Up @@ -1863,7 +1864,7 @@ where
fn address_failed(&mut self, peer_id: PeerId, address: &Multiaddr) {
let key = kbucket::Key::from(peer_id);

if let Some(addrs) = self.kbuckets.entry(&key).value() {
if let Some(addrs) = self.kbuckets.entry(&key).as_mut().and_then(|e| e.value()) {
// TODO: Ideally, the address should only be removed if the error can
// be classified as "permanent" but since `err` is currently a borrowed
// trait object without a `'static` bound, even downcasting for inspection
Expand Down Expand Up @@ -1931,7 +1932,12 @@ where
let (old, new) = (old.get_remote_address(), new.get_remote_address());

// Update routing table.
if let Some(addrs) = self.kbuckets.entry(&kbucket::Key::from(peer)).value() {
if let Some(addrs) = self
.kbuckets
.entry(&kbucket::Key::from(peer))
.as_mut()
.and_then(|e| e.value())
{
if addrs.replace(old, new) {
tracing::debug!(
%peer,
Expand Down Expand Up @@ -2132,7 +2138,7 @@ where
// the addresses of that peer in the k-buckets.
let key = kbucket::Key::from(peer_id);
let mut peer_addrs =
if let kbucket::Entry::Present(mut entry, _) = self.kbuckets.entry(&key) {
if let Some(kbucket::Entry::Present(mut entry, _)) = self.kbuckets.entry(&key) {
let addrs = entry.value().iter().cloned().collect::<Vec<_>>();
debug_assert!(!addrs.is_empty(), "Empty peer addresses in routing table.");
addrs
Expand Down
37 changes: 17 additions & 20 deletions protocols/kad/src/kbucket.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,17 +172,16 @@ where

/// Returns an `Entry` for the given key, representing the state of the entry
/// in the routing table.
pub(crate) fn entry<'a>(&'a mut self, key: &'a TKey) -> Entry<'a, TKey, TVal> {
let index = BucketIndex::new(&self.local_key.as_ref().distance(key));
if let Some(i) = index {
let bucket = &mut self.buckets[i.get()];
if let Some(applied) = bucket.apply_pending() {
self.applied_pending.push_back(applied)
}
Entry::new(bucket, key)
} else {
Entry::SelfEntry
///
/// Returns `None` in case the key points to the local node.
pub(crate) fn entry<'a>(&'a mut self, key: &'a TKey) -> Option<Entry<'a, TKey, TVal>> {
let index = BucketIndex::new(&self.local_key.as_ref().distance(key))?;

let bucket = &mut self.buckets[index.get()];
if let Some(applied) = bucket.apply_pending() {
self.applied_pending.push_back(applied)
}
Some(Entry::new(bucket, key))
}

/// Returns an iterator over all buckets.
Expand Down Expand Up @@ -627,7 +626,7 @@ mod tests {
let other_id = Key::from(PeerId::random());

let mut table = KBucketsTable::<_, ()>::new(local_key, Duration::from_secs(5));
if let Entry::Absent(entry) = table.entry(&other_id) {
if let Some(Entry::Absent(entry)) = table.entry(&other_id) {
match entry.insert((), NodeStatus::Connected) {
InsertResult::Inserted => (),
_ => panic!(),
Expand All @@ -645,10 +644,8 @@ mod tests {
fn entry_self() {
let local_key = Key::from(PeerId::random());
let mut table = KBucketsTable::<_, ()>::new(local_key.clone(), Duration::from_secs(5));
match table.entry(&local_key) {
Entry::SelfEntry => (),
_ => panic!(),
}

assert!(table.entry(&local_key).is_none())
}

#[test]
Expand All @@ -661,7 +658,7 @@ mod tests {
break;
}
let key = Key::from(PeerId::random());
if let Entry::Absent(e) = table.entry(&key) {
if let Some(Entry::Absent(e)) = table.entry(&key) {
match e.insert((), NodeStatus::Connected) {
InsertResult::Inserted => count += 1,
_ => continue,
Expand Down Expand Up @@ -694,10 +691,10 @@ mod tests {
let full_bucket_index;
loop {
let key = Key::from(PeerId::random());
if let Entry::Absent(e) = table.entry(&key) {
if let Some(Entry::Absent(e)) = table.entry(&key) {
match e.insert((), NodeStatus::Disconnected) {
InsertResult::Full => {
if let Entry::Absent(e) = table.entry(&key) {
if let Some(Entry::Absent(e)) = table.entry(&key) {
match e.insert((), NodeStatus::Connected) {
InsertResult::Pending { disconnected } => {
expected_applied = AppliedPending {
Expand Down Expand Up @@ -732,12 +729,12 @@ mod tests {
full_bucket.pending_mut().unwrap().set_ready_at(elapsed);

match table.entry(&expected_applied.inserted.key) {
Entry::Present(_, NodeStatus::Connected) => {}
Some(Entry::Present(_, NodeStatus::Connected)) => {}
x => panic!("Unexpected entry: {x:?}"),
}

match table.entry(&expected_applied.evicted.as_ref().unwrap().key) {
Entry::Absent(_) => {}
Some(Entry::Absent(_)) => {}
x => panic!("Unexpected entry: {x:?}"),
}

Expand Down
3 changes: 0 additions & 3 deletions protocols/kad/src/kbucket/entry.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,6 @@ pub(crate) enum Entry<'a, TPeerId, TVal> {
Pending(PendingEntry<'a, TPeerId, TVal>, NodeStatus),
/// The entry is absent and may be inserted.
Absent(AbsentEntry<'a, TPeerId, TVal>),
/// The entry represents the local node.
SelfEntry,
}

/// The internal representation of the different states of an `Entry`,
Expand Down Expand Up @@ -144,7 +142,6 @@ where
Entry::Present(entry, _) => Some(entry.value()),
Entry::Pending(entry, _) => Some(entry.value()),
Entry::Absent(_) => None,
Entry::SelfEntry => None,
}
}
}
Expand Down

0 comments on commit 294c197

Please sign in to comment.