martinus / unordered_dense

A fast & densely stored hashmap and hashset based on robin-hood backward shift deletion
MIT License
898 stars 72 forks source link

Define mapped_type only for ankerl::unordered_dense::map? #48

Closed Andersama closed 1 year ago

Andersama commented 1 year ago

Is your feature request related to a problem? Please describe. I'm not sure on what the standard says about the mapped_type definition or how it's used, but in the zpp_bits serialization library they're using it to detect the slight variation between the two containers. Intuitively to me it makes sense that void as a value for a key value structure would mean that you're really dealing with a set. Looking at cppreference it appears though the standard might be that mapped_type is specific to maps.

Currently with the set and map being the same template mapped_type gets defined for both, and in zpp_bits this results in that library attempting to create a reference to void in order to iterate over key value pairs, which is a compiler error.

Describe the solution you'd like If mapped_type is specific to maps in the standard use detail::table as a backing implementation where mapped_type is not defined. Then wrap that container for set and map where map has mapped_type defined.

martinus commented 1 year ago

Hi @Andersama! I've got a fix here where mapped_type is only defined for map: https://raw.githubusercontent.com/martinus/unordered_dense/2022-12-no-mapped_type-for-set/include/ankerl/unordered_dense.h

Can you give it a ary if this would work for your serializer? If it does, I'll create a new release with this fix.

Andersama commented 1 year ago

Wish I'd caught this earlier, I'm going to have to wait a few hours before I can test anything.

Got confused, I diffed the wrong files, found what you edited.

// base type for map has mapped_type
template <class T>
struct base_table_type_map {
    using mapped_type = T;
};

// base type for set doesn't have mapped_type
struct base_table_type_set {};

// This is it, the table. Doubles as map and set, and uses `void` for T when its used as a set.
template <class Key,
          class T, // when void, treat it as a set.
          class Hash,
          class KeyEqual,
          class AllocatorOrContainer,
          class Bucket>
class table : public std::conditional_t<std::is_void_v<T>, base_table_type_set, base_table_type_map<T>> {

It looks like it'll work*. I'll let you know when I can recompile my project.

I vaugely remember types with no members might be optimized away, you might want to check your edit doesn't change the size of the resulting struct.

Andersama commented 1 year ago

Looks like it works, all good here with the edit.

martinus commented 1 year ago

I vaugely remember types with no members might be optimized away

You probably think of empty base class optimization, I'm pretty sure all compilers do this

Thanks for testing, I'll create a new release

Andersama commented 1 year ago

That's it. The tricky thing is I've had a few structs change layout from debug and release and I think empty base classes might be one of those things that gets toggled on and off. Fairly confident modern ones will for release. I'm going to have to check. I just didn't want to suggest something and then end up bloating your hash table.