Slot map - Add unit tests, conform operator[]() to comment, add at_unchecked(), add key()

[p-groarke]

• Adds a few missing unit tests.

• operator[] didn't conform to the comment. Moved completely unchecked behavior to at_unchecked and modified operator[] to match comment. It has a generation check, but no out-of-bounds check.

• As mentioned in [slot_map] Can you get a key using an iterator? #141, add a key api to find the key of a given element.

• Renamed key_size_type to key_index_type.

[Quuxplusone]

The original comment may be unnecessarily confusing, but I believe the original comment and code are correct, here. "operator[] has a check, and if that check fails, you get UB" is basically the same thing as "operator[] has no check."

(By analogy to vector, the proper name for the unchecked/UB-prone version of at() is operator[].)

[p-groarke]

IMHO it isn't the same, as your debug build will catch the undefined behavior with debug iterators. Just as accessing vector out-of-range is caught in debug.

[Quuxplusone]

Hmm. I had not been thinking about the observable behavior re debug iterators. That's a thought-provoking wrinkle.

So in master right now there are two ways to index into a slot_map: sm.at(key) (throws on key-not-found) and sm[key] (UB on key-not-found). And there are two ways to index into a vector: vec.at(idx) (throws on out-of-range) and vec[idx] (UB on out-of-range, except on MSVC with _ITERATOR_DEBUG_LEVEL != 0, where it calls _STL_REPORT_ERROR("vector subscript out of range")).

So I think you're asking for sm[key] to go via a codepath that, when _ITERATOR_DEBUG_LEVEL != 0, will reliably trigger _STL_REPORT_ERROR("something-or-other") on key-not-found. Which your PR does implement, because *end() will trigger _STL_REPORT_ERROR.

However, I don't want regular users of sm[key] (outside of debug mode) to pay for the check on your line 138:

    if (get_generation(*slot_iter) != get_generation(key)) {
        return *end();
    }

I guess if you just enclosed those three lines in #if _ITERATOR_DEBUG_LEVEL != 0, then that would accomplish your goal. Feels icky to me, though.

[p-groarke]

Yeah I find that icky as well. I think this is worth hashing out, because it speaks to the fundamental understanding of slot_map and its behavior.

For me, slot_map is a map. If I give a bad key to a map, I expect things to "break". Returning an unrelated object (the same one every time), isn't undefined behavior. It is quite predictable, you get the last object that was at that slot. I guess you could argue that any behavior is undefined behavior, but that is a stretch ;)

Lets say you completely ignore the generation here, then the map accepts non-existant keys and happily returns the object at that slot. Doesn't this completely redefine what a slot_map is supposed to be?

Performance-wise, the slot_map excels at iterating multiple objects in the underlying container. I don't believe a single if is problematic, especially if you compare to a hash + collision you might get from unordered_map<size_t, T>. Which is what a slot_map replaces.

[Quuxplusone]

Returning an unrelated object (the same one every time), isn't undefined behavior. It is quite predictable, you get the last object that was at that slot.

That is, you get a reference to a destroyed object [EDIT: only if the map has been cleared, I guess], or a reference to an unrelated object (if the slot has been reused), or a segfault (if the underlying vector has been reallocated and the memory not reused for a different object yet), or a plain old wild pointer (if the memory has been reused). Similarly, with std::vector::operator[], when you access an invalidated iterator, you get a reference to a destroyed object, or a reference to an unrelated object (if the slot has been reused), or a segfault or a wild pointer. All of these possibilities are conforming manifestations of "undefined behavior."

In MSVC's debug mode, you pay a hefty runtime cost to detect invalid accesses and make them produce predictable assert-like behavior. In release mode, all the cost goes away.

I wouldn't strongly object to adding such a "debug mode" to slot_map. But I would want to make sure that in "release mode," all the cost goes away.

Lets say you completely ignore the generation here, then the map accepts non-existant keys and happily returns the object at that slot. Doesn't this completely redefine what a slot_map is supposed to be?

Indexing into a core-language array with a[10000] happily returns the object 10000*sizeof(a[0]) bytes beyond the start of a. Does that "completely redefine" what an array is supposed to be? I say "no." Undefined behavior does sometimes do the "expected" thing. But it has the freedom to do the "wrong" thing, too, and we exploit that freedom to gain a little extra runtime performance.

[p-groarke]

OK fair enough. I'll remove that part of the PR. Will squash as well.

I guess I should ask one last time, would you be willing to add an assert here? I know you didn't want to add a dependency on <cassert>, but I feel like it is fully justifiable here.

[p-groarke]

@Quuxplusone I updated the PR with your comments. I removed at_unchecked, I left the changes for operator[]. I think asserting would be ok with the old behavior, but as you mentioned before, adding a dependency to the cassert header isn't ideal. I don't think the subscript operator should happily and quietly return the wrong element, without a way to notify the user. The current implementation should fail in debug, without requiring asserts, and mimics accessing a vector out-of-range.

I couldn't use KeysAreEqual as the test runs on std::pair keys. So I added a comment to clarify + used hidden friends. Should be a good compromise.