bennybp / BPHash

Simple hashing library for C++
BSD 3-Clause "New" or "Revised" License
3 stars 6 forks source link

Handling overloads for types which extend STL containers #3

Open wavefunction91 opened 3 years ago

wavefunction91 commented 3 years ago

Currently, defining a custom hasher for types which extent STL containers (e.g. std::vector) lead to an ambiguous overload in hash_single_ delegating between the user defined hash implementation and the free-standing hash_object defined for its STL instantiation.

e.g.

struct A {
  int x;
  void hash( bphash::Hasher& h ) const { h( x ); }
};

struct B : public std::vector<A> {
  double y;
  void hash( bphash::Hasher& h ) const { 
    h( dynamic_cast<const std::vector<A>&>(*this), y ); 
  }
};

Generally, the delegation to B::hash and hash_object( const std::vector<A>& ) is ambiguous in the delegation for hash_single_

Without knowing for sure your motivations, my guess is that you would want to prefer the user defined hash (B::hash in this case) rather than the hash_object implementation.

I have a fix for this which SFINAE disables the hash_object path when the member function implementation exists. I'm more than happy to PR it, but it depends on #2 (this was done in the context of NWX which pulls that PR). let me know how you'd like to proceed or if you have other opinions on the preference to resolve the ambiguous function call.

ryanmrichard commented 3 years ago

It's tangent to the actual issue, and you may already be aware, but you need to be very careful publicly inheriting from STL containers. The dtors are not virtual; so you'll run into all sorts of problems if you attempt to use the classes in typical polymorphic scenarios (i.e., passing the objects around via the base class).

wavefunction91 commented 3 years ago

Yes, I am aware, I won't get into the specific use case because it's completely tangent to this issue (check Libint's BasisSet definition)

Maybe the original issue was posed incorrectly - this problem will arise in any case where a Base class is handled by hash_object and the Derived class defines a Derived::hash.

On Sun, May 2, 2021, 07:26 Ryan Richard @.***> wrote:

It's tangent to the actual issue, and you may already be aware, but you need to be very careful publicly inheriting from STL containers. The dtors are not virtual; so you'll run into all sorts of problems if you attempt to use the classes in typical polymorphic scenarios (i.e., passing the objects around via the base class).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bennybp/BPHash/issues/3#issuecomment-830817781, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPLZPN5Z35VJKDRJE5PMJDTLVOJ3ANCNFSM435QZQRQ .

ryanmrichard commented 3 years ago

FWIW, I don't think @bennybp wants to support this library anymore so it may be time to look into another hashing library or to have him give one of us admin privileges on this repo so we can improve it.