Inconsistency between documentation and code implementation

[YichiZhang0613]

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();
    }

[gyscos]

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 if i > 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().

[YichiZhang0613]

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!

[gyscos]

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.

[gyscos]

I ended up adding a couple of variants that return None rather than panicking when accessing out of bound items.