cursive
cursive copied to clipboard
Inconsistency between documentation and code implementation
I noticed a possible panic due to inconsistency between documentation and code implementation in cursive-main/cursive-core/src/views/linear_layout.rs. The details can be found in the following code. The code does not check whether i is out of bounds before use it directly.
/// Panics if `i >= self.len()`.
pub fn set_weight(&mut self, i: usize, weight: usize) {
self.children[i]._weight = weight;
}
The similar situation can be found in cursive-main/cursive-core/src/views/list_view.rs
/// Panics if `id >= self.len()`.
pub fn row_mut(&mut self, id: usize) -> &mut ListChild {
&mut self.children[id]
}
Besides I think this documentation in cursive-main/cursive-core/src/views/linear_layout.rs is not complete, which should be "Panics if i >= self.len()
" instead of "Panics if i > self.len()
"
/// Panics if `i > self.len()`.
pub fn insert_child<V: IntoBoxedView + 'static>(&mut self, i: usize, view: V) {
self.children.insert(
i,
Child {
view: view.into_boxed_view(),
required_size: Vec2::zero(),
last_size: Vec2::zero(),
_weight: 0,
},
);
self.invalidate();
}
Thanks for the report!
For set_weight
:
The code does not check whether i is out of bounds before use it directly.
self.children[i]
is guaranteed to panic first here, so we just document this panic case.
For insert_child
:
should be "Panics if
i >= self.len()
" instead of "Panics ifi > self.len()
"
Insertion at the end of the list is actually possible - it's effectively appending the entry to the list. This is why self.children.insert(i, ...)
only panics if i > self.children.len()
.
Hi,
Many thanks for your help.
I understand your point, but I do not think“out-of-bound” crash is the behavior we expect.
The ideal situation is to inform the "user" what should happen if it is out-of-bound.
Thus, I think we may use the get and get_mut to check whether the index is in the Vec.
Thanks!
In each of these cases, users could check first if i > layout.len()
manually, and react accordingly.
We could add variants that return Option<...>
instead of panicking, but it might clutter the API a bit.