kernel/collections/queue: rename remove_first to remove_first_matching
#4033
Conversation
…hing` `Queue`'s `dequeue_specific` was renamed to `remove_first` in ab293c5 ("Rename dequeue_specific to remove_first"). While I don't disagree with the fact that `dequeue_specific` is an oxymoron and a bad name, arguably `remove_first` carries its own baggage. We're talking about collections where "first" has meaning, often used interchangably with "head". Looking over the code in tock#3577 I was quite surprised with the complexity of this method's implementation in `RingBuffer`, for what I thought would just be a `dequeue` operation---removing the _first_ / _head_ element. This is an effort to clear up that confusion. This is not a hill worth dying on for me, so if anyone has strong feelings otherwise, feel free to close this.
There was a problem hiding this comment.
Wait dequeue_specific() changed name? Why does "to dequeue" have to mean the front of the queue?
I think the fix is to go back to dequeue_specific().
remove_firstis a general operation for list-like structures and better follows conventions around naming such operations.
But this isn't a list-like structure, it is a queue.
|
Also I agree with Leon that |
|
I guess |
|
I think remove_first where the argument is a lambda that takes an element and returns a bool, pretty clearly matches the implementation, which removes the first element that matches the predicate. But I don't care very much. |
|
|
|
Again, I'm not dying on this hill, but |
|
Why do we have both verbs? Remove and Dequeue? |
Because they mean different things. To refer to your original comments:
Because that is literally the definition of a queue (https://en.wikipedia.org/wiki/Queue_(abstract_data_type))
Exactly. This method is entirely breaking the abstraction that this thing is a queue (a queue can only have the head removed), and exposing the fact that it is actually a list-like structure, than has an order of elements that can be removed otherwise. Arguably, if we were interested in having more formalism here, we might have this structure and a number of traits: trait Queue<T> {
fn enqueue(&self, item: T);
fn dequeue(&self) -> Option<T>;
}
trait ListLike<T> {
fn remove_first_matching(...) -> Option<T>;
fn remove_at(&self, i: usize) -> Option<T>;
fn insert(&self, i: usize, item: T);
...
}And this structure implements all of them. But we probably don't need to have a bunch of different traits for just one data structure, so we can just put those methods in the impl directly, and name them based on the kind of interface they relate to. |
Pull Request Overview
Queue'sdequeue_specificwas renamed toremove_firstin ab293c5 ("Rename dequeue_specific to remove_first").While I don't disagree with the fact that
dequeue_specificis an oxymoron and a bad name, arguablyremove_firstcarries its own baggage. We're talking about collections where "first" has meaning, often used interchangably with "head". Looking over the code in #3577 I was quite surprised with the complexity of this method's implementation inRingBuffer, for what I thought would just be adequeueoperation---removing the first / head element.This is an effort to clear up that confusion. This is not a hill worth dying on for me, so if anyone has strong feelings otherwise, feel free to close this.
Testing Strategy
N/A
TODO or Help Wanted
N/A
Documentation Updated
Updated the relevant files inor no updates are required./docs,Formatting
make prepush.