Re: [PATCH 1/4] Add an abstract incremental hash data type

[Trevor Saunders <tsaunders at mozilla dot com>]

On Thu, Jul 17, 2014 at 06:36:31AM +0200, Andi Kleen wrote:
> On Wed, Jul 16, 2014 at 10:40:53PM -0400, Trevor Saunders wrote:
> > 
> > > + public:
> > > +
> > > +  /* Start incremential hashing, optionally with SEED.  */
> > > +  void begin (hashval_t seed = 0)
> > > +  {
> > > +    val = seed;
> > 
> > why isn't this the ctor?
> 
> It's standard for hash classes to have explicit begin()/end().
> All the existing ones I've seen work this way.

 I only know of one vaguelly similar thing
 http://mxr.mozilla.org/mozilla-central/source/mfbt/SHA1.h#37  which
 doesn't do that, and a bunch of people doing something doesn't
 necessarily mean it makes sense.  Now there may be a good reason it
 does make sense, but unless these other people need begin() to be
 fallible I don't see it.

> > > +  /* Add unsigned value V.  */
> > > +  void add_int (unsigned v)
> > > +  {
> > > +    val = iterative_hash_hashval_t (v, val);
> > > +  }
> > 
> > istm this is a great spot to provide a bunch of overloads of just add()
> > and let the compiler pick the appropriate one for your type.
> 
> Sorry I'm not into code obfuscation. With hashing it's far better
> to work with explicit visible types instead of invisible magic.

if that were true I'd expect you'd see lots of cases of people using a
different hash function than the type of the expression being passed,
but a quick look at the later patches didn't show me any of those.
Not repeating the type of something is hardly obfiscation, and in most
cases there's only one sane function to call for a given expression.

but I guess its easy enough to change later if somebody gets really
annoyed by it so whatever.

Trev

> 
> -Andi