Use Result in several places and solve double deletion problem

[torkleyy]

Not meant to be merged as is

Fixes #94

[torkleyy]

Okay, I think this is ready for review now.

[csherratt]

I looks ok.

[csherratt]

Could this be an enum or a &'static string?

[Aceeri]

What kind of breakage would occur without this?

[torkleyy]

If a client matches against all the variants and we add one, it would cause an error about non-exhaustive matching. By having this additional variant which should never be used, the user is forced to match against _. This is a very common pattern (even the standard library uses it, see https://doc.rust-lang.org/src/core/sync/atomic.rs.html#220) and there's an RFC about creating an attribute #[non_exhaustive] for just this case.

[kvark]

Looks reasonable. I have 2 concerns:

1. some clients may be better at tracking the entity lifetimes, so we should provide an unsafe way to delete those without extra checks
2. better have an enum for the action, like @csherratt noted

[torkleyy]

@kvark A deletion without checks wouldn't really be unsafe, the semantics are just not clearly defined (whether it panics or it's silently ignored). Benchmarking both versions, I cannot see any difference in performance (it may be the case that other checks are eliminated, idk). So if the difference is smaller than 1 ns/iter, would it still make sense to add a second variant?

[kvark]

A deletion without checks wouldn't really be unsafe, the semantics are just not clearly defined (whether it panics or it's silently ignored)

Even better then - let's have a safe version that ignores the errors. For ergonomics and this undetectable bit of performance ;)

[torkleyy]

So should the _unchecked version not do the check or just silently ignore a deletion of a dead entity?

[torkleyy]

I'm assuming the latter right now, but it's not clear to me. I added these Results because before the behavior was pretty much undefined.

Here's the previous behavior (without checks):

• Both kill and kill_atomic don't check the generation of the entity. Thus, an entity with an old id could be used to delete an entity with a newer id.
• kill would cause a panic in debug mode if the entity has been deleted by kill or a merged kill_atomic
• kill_atomic would cause a panic in debug mode if the entity has been deleted by kill or a merged kill_atomic
• a second kill_atomic would be silently ignored if called twice on the same entity, but in the same frame

Given this confusing behavior, I thought that a clear check would be better. So I'm not sure if it makes sense to offer a version without a check at all (as opposed to checking and ignoring).

[kvark]

Ok, that's convincing enough. Thank you!
bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push