This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 1/4] Add an abstract incremental hash data type
- From: Trevor Saunders <tsaunders at mozilla dot com>
- To: Andi Kleen <andi at firstfloor dot org>
- Cc: gcc-patches at gcc dot gnu dot org, Andi Kleen <ak at linux dot intel dot com>
- Date: Thu, 17 Jul 2014 21:08:52 -0400
- Subject: Re: [PATCH 1/4] Add an abstract incremental hash data type
- Authentication-results: sourceware.org; auth=none
- References: <1405488709-12677-1-git-send-email-andi at firstfloor dot org> <1405488709-12677-2-git-send-email-andi at firstfloor dot org> <20140717024053 dot GA23343 at tsaunders-iceball dot corp dot tor1 dot mozilla dot com> <20140717043631 dot GD18735 at two dot firstfloor dot org>
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