Skip to content

Explictly call Ord::cmp to compare priorities #85

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.

Sign up for GitHub

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

Open
Zabot wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Explictly call Ord::cmp to compare priorities #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 34 additions & 11 deletions src/double_priority_queue/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,10 @@ where
///
/// Computes in **O(log(N))** time.
pub fn push_increase(&mut self, item: I, priority: P) -> Option<P> {
if self.get_priority(&item).map_or(true, |p| priority > *p) {
if self
.get_priority(&item)
.map_or(true, |p| priority.cmp(p).is_gt())
{
self.push(item, priority)
} else {
Some(priority)
Expand Down Expand Up @@ -705,7 +708,10 @@ where
///
/// Computes in **O(log(N))** time.
pub fn push_decrease(&mut self, item: I, priority: P) -> Option<P> {
if self.get_priority(&item).map_or(true, |p| priority < *p) {
if self
.get_priority(&item)
.map_or(true, |p| priority.cmp(p).is_lt())
{
self.push(item, priority)
} else {
Some(priority)
Expand Down Expand Up @@ -901,15 +907,20 @@ where
.0;

if unsafe {
self.store.get_priority_from_position(i) < self.store.get_priority_from_position(m)
self.store
.get_priority_from_position(i)
.cmp(self.store.get_priority_from_position(m))
.is_lt()
} {
self.store.swap(i, m);
if i > r {
// i is a grandchild of m
let p = parent(i);
if unsafe {
self.store.get_priority_from_position(i)
> self.store.get_priority_from_position(p)
self.store
.get_priority_from_position(i)
.cmp(self.store.get_priority_from_position(p))
.is_gt()
} {
self.store.swap(i, p);
}
Expand Down Expand Up @@ -943,15 +954,20 @@ where
.0;

if unsafe {
self.store.get_priority_from_position(i) > self.store.get_priority_from_position(m)
self.store
.get_priority_from_position(i)
.cmp(self.store.get_priority_from_position(m))
.is_gt()
} {
self.store.swap(i, m);
if i > r {
// i is a grandchild of m
let p = parent(i);
if unsafe {
self.store.get_priority_from_position(i)
< self.store.get_priority_from_position(p)
self.store
.get_priority_from_position(i)
.cmp(self.store.get_priority_from_position(p))
.is_lt()
} {
self.store.swap(i, p);
}
Expand All @@ -970,7 +986,10 @@ where
let parent = parent(position);
let parent_priority = unsafe { self.store.get_priority_from_position(parent) };
let parent_index = unsafe { *self.store.heap.get_unchecked(parent.0) };
position = match (level(position) % 2 == 0, parent_priority < priority) {
position = match (
level(position) % 2 == 0,
parent_priority.cmp(priority).is_lt(),
) {
// on a min level and greater then parent
(true, true) => {
unsafe {
Expand Down Expand Up @@ -1008,7 +1027,9 @@ where
let mut grand_parent = Position(0);
while if position.0 > 0 && parent(position).0 > 0 {
grand_parent = parent(parent(position));
(unsafe { self.store.get_priority_from_position(grand_parent) }) > priority
(unsafe { self.store.get_priority_from_position(grand_parent) })
.cmp(priority)
.is_gt()
} else {
false
} {
Expand All @@ -1027,7 +1048,9 @@ where
let mut grand_parent = Position(0);
while if position.0 > 0 && parent(position).0 > 0 {
grand_parent = parent(parent(position));
(unsafe { self.store.get_priority_from_position(grand_parent) }) < priority
(unsafe { self.store.get_priority_from_position(grand_parent) })
.cmp(priority)
.is_lt()
} else {
false
} {
Expand Down
28 changes: 21 additions & 7 deletions src/priority_queue/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,10 @@ where
///
/// Computes in **O(log(N))** time.
pub fn push_increase(&mut self, item: I, priority: P) -> Option<P> {
if self.get_priority(&item).map_or(true, |p| priority > *p) {
if self
.get_priority(&item)
.map_or(true, |p| priority.cmp(p).is_gt())
{
self.push(item, priority)
} else {
Some(priority)
Expand Down Expand Up @@ -579,7 +582,10 @@ where
///
/// Computes in **O(log(N))** time.
pub fn push_decrease(&mut self, item: I, priority: P) -> Option<P> {
if self.get_priority(&item).map_or(true, |p| priority < *p) {
if self
.get_priority(&item)
.map_or(true, |p| priority.cmp(p).is_lt())
{
self.push(item, priority)
} else {
Some(priority)
Expand Down Expand Up @@ -762,12 +768,16 @@ where
let mut largestp = unsafe { self.store.get_priority_from_position(i) };
if l.0 < self.len() {
let childp = unsafe { self.store.get_priority_from_position(l) };
if childp > largestp {
if childp.cmp(largestp).is_gt() {
largest = l;
largestp = childp;
}

if r.0 < self.len() && unsafe { self.store.get_priority_from_position(r) } > largestp {
if r.0 < self.len()
&& unsafe { self.store.get_priority_from_position(r) }
.cmp(largestp)
.is_gt()
{
largest = r;
}
}
Expand All @@ -780,14 +790,16 @@ where
l = left(i);
if l.0 < self.len() {
let childp = unsafe { self.store.get_priority_from_position(l) };
if childp > largestp {
if childp.cmp(largestp).is_gt() {
largest = l;
largestp = childp;
}

r = right(i);
if r.0 < self.len()
&& unsafe { self.store.get_priority_from_position(r) } > largestp
&& unsafe { self.store.get_priority_from_position(r) }
.cmp(largestp)
.is_gt()
{
largest = r;
}
Expand All @@ -802,7 +814,9 @@ where
let mut parent_position = Position(0);
while if position.0 > 0 {
parent_position = parent(position);
(unsafe { self.store.get_priority_from_position(parent_position) }) < priority
(unsafe { self.store.get_priority_from_position(parent_position) })
.cmp(priority)
.is_lt()
} else {
false
} {
Expand Down
35 changes: 35 additions & 0 deletions tests/double_priority_queue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1275,6 +1275,41 @@ mod doublepq_tests {
);
}
}

#[test]
fn partial_cmp_not_called() {
use std::cmp::{Ordering, PartialOrd};

#[derive(Debug, PartialEq, Eq, Hash, Ord)]
struct PanicPartial(i64);

// This is an invalid implementation of PartialOrd according to
// the docs in `std::cmp::PartialOrd`, since Ord is also implemented,
// this should always return Some(Ord::cmp(self, other)). Instead this
// function panics as a way to ensure that we don't accidently
// use PartialOrd::partial_cmp when we should be using Ord::cmp
// instead. Enforcing the explicit use of Ord::cmp lets us rely on
// the compiler instead of the convention that PartialOrd::partial_cmp
// _should_ call Ord::cmp
impl PartialOrd for PanicPartial {
fn partial_cmp(&self, _other: &Self) -> Option<Ordering> {
panic!("partial_cmp should not be called");
}
}

let mut dpq = DoublePriorityQueue::new();
dpq.push(0, PanicPartial(100));
dpq.push(1, PanicPartial(200));
dpq.push(2, PanicPartial(150));
dpq.push_increase(2, PanicPartial(300));
dpq.push_decrease(2, PanicPartial(0));

// These asserts are redundant since this behavior is tested elsewhere, we're
// mainly just interested in not panicking for this test.
assert_eq!(dpq.pop_min(), Some((2, PanicPartial(0))));
assert_eq!(dpq.pop_max(), Some((1, PanicPartial(200))));
assert_eq!(dpq.pop_min(), Some((0, PanicPartial(100))));
}
}

#[cfg(all(feature = "serde", test))]
Expand Down
35 changes: 35 additions & 0 deletions tests/priority_queue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1143,6 +1143,41 @@ mod pqueue_tests {
assert_eq!(removed_priority, 200);
assert!(!pq.contains(&bob_view));
}

#[test]
fn partial_cmp_not_called() {
use std::cmp::{Ordering, PartialOrd};

#[derive(Debug, PartialEq, Eq, Hash, Ord)]
struct PanicPartial(i64);

// This is an invalid implementation of PartialOrd according to
// the docs in `std::cmp::PartialOrd`, since Ord is also implemented,
// this should always return Some(Ord::cmp(self, other)). Instead this
// function panics as a way to ensure that we don't accidently
// use PartialOrd::partial_cmp when we should be using Ord::cmp
// instead. Enforcing the explicit use of Ord::cmp lets us rely on
// the compiler instead of the convention that PartialOrd::partial_cmp
// _should_ call Ord::cmp
impl PartialOrd for PanicPartial {
fn partial_cmp(&self, _other: &Self) -> Option<Ordering> {
panic!("partial_cmp should not be called");
}
}

let mut pq = PriorityQueue::new();
pq.push(0, PanicPartial(100));
pq.push(1, PanicPartial(200));
pq.push(2, PanicPartial(150));
pq.push_increase(2, PanicPartial(300));
pq.push_decrease(2, PanicPartial(0));

// These asserts are redundant since this behavior is tested elsewhere, we're
// mainly just interested in not panicking for this test.
assert_eq!(pq.pop(), Some((1, PanicPartial(200))));
assert_eq!(pq.pop(), Some((0, PanicPartial(100))));
assert_eq!(pq.pop(), Some((2, PanicPartial(0))));
}
}

#[cfg(all(feature = "serde", test))]
Expand Down
Loading