Add to SetOfVec at lexicographically correct position

[bkstein]

Motivation

When adding an element to a SetOfVec, lexicographical ordering must be respected. This is required, when the content is DER encoded. Current solution is to forbid adding elements, that are not greater than all exisiting elements.
This PR adds new elements in the correct position, which makes it possible to add elements to existing SetOfVecs at any time in any order.

Overhead?

The overhead introduced by this implementation is not really an overhead, because otherwise, the user of this crate would have to sort. And due to the current order check, there is no way around sorting anyway.

SetOf

The alloc-free SetOf was not changed. But SetOf::add() could also be improved, I think.

[tarcieri]

Perhaps we need to better document/advertise this, but there's a TryFrom<Vec<T>> impl for SetOfVec<T> which takes ownership of a Vec and sorts it in-place.

That seems a lot more efficient than your proposed change.

[bkstein]

Yeah, damn, I overlooked that. But this doesn't really help, if you get a SetOfVec from somewhere and want to add an element. add() is not too useful in these cases...
But this was only a proposal. I can live with TryFrom<Vec<T>>, so we can close this PR, if you like.

[bkstein]

@tarcieri A test in the x509 crate tests the order of adding to SetOfVec. I updated the test, but it looks like CI takes the wrong version of the der crate. I couldn't figure out how to fix that.
However, if you would rather reject this PR, it might not be worth fixing this. Please feel free to close it. Thanks!

[tarcieri]

CI should be patched to use the in-repo der crate here: https://github.com/RustCrypto/formats/blob/master/Cargo.toml#L44

I agree the existing method's ergonomics aren't great, but I'm a bit worried changing it like this creates an API which though convenient to use has some pathological corner cases performance-wise.

If the primary use case is adding an item to an existing set, does that really make sense via in-place mutation, or would you normally Clone an existing SetOfVec first? If it's the latter, it might make more sense to have a method like:

pub fn append(&self, new_elem: T) -> Result<Self>

...which can be constructed by moving the inner Vec out, pushing new_elem into it, then using the existing TryFrom implementation to sort the Vec and return a new SetOfVec.

[tarcieri]

Also der_sort is using insertion sort which is highly efficient on mostly-in-order data anyway, so you should be able to use that same strategy for an in-place mutation as well: push the value onto the inner Vec and call der_sort, which should make for a simpler implementation (and a potentially faster one?)

Edit: this approach could also make for a general extend which takes an iterator, calls the underlying Extend impl on Vec, then sorts the Vec. A single-item append could be composed in terms of extend.

[tarcieri]

Went ahead and impl'd extend and from_iter: #1065

[tarcieri]

I have an alternative proposal in #1067.

It also avoids making breaking changes.

[tarcieri]

Went ahead and merged #1067, which addresses the same issue as this PR but with a simpler implementation.

@bkstein can you try it out and see if it addresses your issue? If so I can cut a release of the der crate.

[bkstein]

@tarcieri #1067 solves the issue, too. So it's OK from my side. Perhaps you could mention the difference between add() and extend() with respect to DER ordering?

[tarcieri]

add is just a deprecated alias for insert_ordered, which does have a comment that it expects ordered inputs. It could be a little better I guess.

[tarcieri]

Re: extend, it accepts an iterator like the Extend trait (but it can't actually impl that trait because it's fallible)