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

From: Julien Blanc (julien.blanc_at_[hidden])
Date: 2024-12-06 15:54:17

6 décembre 2024 à 14:24 "Andrey Semashev via Boost" a écrit:

> >  
> >  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

I think i start to understand what you mean. In what i have in mind :

* hash algorithm implementation and interface do not depend on boost::hash2
  (this is the case currently)
* custom object hashing does depend on boost::hash2 (this is also the case 
  currently)

>From your examples, it seems to me that you would like custom object hashing
to also be independant from boost::hash2. Is that accurate?

> > > 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

What i had in mind was more:

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

Notice the namespace is still boost::hash2. I expect it to work whatever std::fnv1a_64
finally chose as its interface.

On a broader view, Claudio's made a very valid point, which made me realize that the
current review goals may be a bit unusual. The usual review focus on finding the best
suitable interface for the library. However, i don't think such interface exists:

* span<void const> is not available, even with a bleeding edge compiler.
* support for older C++ versions mandates support for void* + size pointer as separate
arguments (no span vocabulary type), which we know will be flagged by compilers as unsafe

So, in my opinion, part of the review should also focus on how will the library 
evolve toward a safer/modern version, with minimal burden for the library user. 
Basically we know that we will deliver an interface that is outdated by current
standards, but that we have very valid reasons to do that (these reasons would 
not hold if we were targetting std). We should think about of we handle the
cohabitation with a safer interface, and how we provide a smooth transition 
path to users, taking full advantage of available compiler features, so that 
when they update their code to newer standards and safer patterns, boost::hash2 is
not a concern and just works flawlessly.

I understand that it is asking a lot to the library authors.

Regards,

Julien