Re: [hash2][review] Early review (due to holidays)

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2024-12-06 13:24:22

On 12/6/24 14:13, Julien Blanc via Boost wrote:
> 6 décembre 2024 à 11:15 "Andrey Semashev via Boost" a écrit:
> 
>> A hasher must expose a common public interface anyway to be compatible
>> with hash_append
> 
> This is stricter than what is actually required. hash_append can be made to 
> work with hashers having a different interface, as long as they follow a concept 
> known to hash_append / hash_update. This concept does not need to rely on
> types defined by boost::hash2, the dependency is the other way (boost::hash2
> must include a specialization for the interface of the hasher).
> 
>> , and at this point why not use the interface in the
>> user types hash functions directly? What value a free hash_append
>> function would bring? The only thing that comes to mind is it could have
>> a more flexible interface, as Vinnie suggested, to keep the hasher
>> interface minimal, but that's about it.
> 
> That is imho largely worth it. This also gives a lot more flexibility / room 
> for evolution. There is an order of magnitude between the number of usages 
> of the hashing api, and the number of hashers used in a project. Changing the
> hashing api means changing every call to hash_append (resp hash_update). 
> Changing the hasher concept only means changing a few files.
> 
> But, as i said before, you can even support multiple hasher concepts in
> hash_append / hash_update, so changing the hasher recommended api can be made
> while keeping old hasher api still working. Something that cannot be done if 
> you talk directly to the hasher.
> 
>> I do think though that whether or not a free hash_append function is
>> provided, using the hasher directly through its public interface should
>> be allowed. 
> 
> I don't suggest to make changes to specifically forbid such use. Just that
> the nominal / advertised usage uses a thin wrapper (which, i believe, would
> be fully inlined and not result in additional runtime cost).
> 
>> This would allow the user types' hash functions not depend
>> on Boost.Hash2 and therefore
> 
> This is not required by what i suggest.
> 
>> make support for hashing more lightweight
> 
> I expect the wrapper to incur no additional runtime cost (over manual conversions 
> that would be done at call site instead).

I also had compile-time dependencies in mind when I said "lightweight".
To take the example from the docs:

https://pdimov.github.io/hash2/doc/html/hash2.html#hashing_objects_user_defined_types

  class X
  {
  private:
    std::string a;
    int b;

  // ...
  public:
    template<class Hash, class Flavor>
    friend void tag_invoke( boost::hash2::hash_append_tag const&,
      Hash& h, Flavor const& f, X const& v )
    {
      boost::hash2::hash_append(h, f, v.a);
      boost::hash2::hash_append(h, f, v.b);
    }
  };

This requires one to depend on Boost.Hash2 for hash_append_tag and
hash_append. I would prefer if there was a way (properly documented and
supported) to avoid this dependency entirely. I.e. so that something
like this is a working and advertised way to add support for hashing to
your type:

  class X
  {
  private:
    std::string a;
    int b;

  // ...
  public:
    template<class Hash, class Flavor>
    friend void hash_append( Hash& h, Flavor const& f, X const& v )
    {
      h.update(f, v.a);
      h.update(f, v.a.size());
      h.update(f, v.b);
    }
  };

  X x;
  boost::hash2::fnv1a_64 h;
  boost::hash2::hash_append(h, {}, x); // works

>> and potentially compatible with std::hash2, if one ever comes to light.
> 
> I do not see how this is related.

What I meant by this is that in the above example Hash could become
std::fnv1a_64 or whatever, if the std library adopts the proposal, and
the existing hashing infrastructure will automatically support it. This
is one of the main selling points of the library/proposal, after all.

  X x;
  std::fnv1a_64 h;
  std::hash_append(h, {}, x); // works, with no changes to X