Add in missing nullptr check when calling std::slice::from_raw_parts

[maspe36]

Seems like back in #407 we added a conditional to not call write_bytes() if the internal data buffer of a Sequence was a nullptr.

It is true that it is unsafe to pass a nullptr to write_bytes

the pointer must be non-null and properly aligned.

But this breaks our Sequence abstraction because now an empty sequence has a nullptr for its buffer. So later, when we may call from_raw_parts(), we panic if our Sequence is empty.

data must be non-null and aligned even for zero-length slices.

So this PR is a small hack for #411, but doesn't fundamentally address the issue.

---

Taking a deeper look at the BoundedSequence and Sequence structs, I think we should refactor them to use something like RawVec (The RawVec impl in the std lib actually uses Unique which is currently unstable 😞 ). We know the type that a Sequence or BoundedSequence is constructed with, so there should be no reason our internal buffer isn't non-zero and aligned.

Vec::<i32>::default().is_empty() doesn't panic so Sequence::<i32>::default().is_empty() shouldn't panic.

[maspe36]

Actually, now that I'm looking at this, I'm not sure this would work. If someone started pushing to this mutable slice self.data would never get updated, so this likely would compile but exhibit incorrect runtime behavior.

[jhdcs]

Is it possible to write a unit test for this scenario? Just to make sure that it's working, and also as a warning sign in case future changes cause it to break?

[maspe36]

Is it possible to write a unit test for this scenario?

Good point, added a unit test for the base cases. But also, just digging around the slice API, doesn't seem like this is really a concern to mutate the slice as its empty, and the only mutation that can be done will be with the fixed size.

i.e.

let mut slice = Sequence::<i32>::default().as_mut_slice();

// I can't do anything useful with slice now
// slice[0] = 42 will segfault because the len of the slice is zero.
// I actually don't think it matters, its just not really idiomatic rust

[maspe36]

Taken from this example in the stdlib

https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html#ffi-handling-null-pointers

[esteve]

@maspe36 good catch! Thanks.

[jhdcs]

I think this should probably work. After thinking about it a bit more, I don't think the mutating slice thing should be an issue since I think the borrow checker should protect against that. Right?

[maspe36]

Yup, @jhdcs that was the same realization I came to. I've been writing too much C++ :)