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: Richard Biener <richard dot guenther at gmail dot com>
- To: Trevor Saunders <tsaunders at mozilla dot com>
- Cc: Andi Kleen <andi at firstfloor dot org>, GCC Patches <gcc-patches at gcc dot gnu dot org>, Andi Kleen <ak at linux dot intel dot com>
- Date: Wed, 23 Jul 2014 11:37:05 +0200
- 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> <20140718010852 dot GA8100 at tsaunders-iceball dot corp dot tor1 dot mozilla dot com>
On Fri, Jul 18, 2014 at 3:08 AM, Trevor Saunders <tsaunders@mozilla.com> wrote:
> 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.
I agree with Trevor here. Please make begin() the constructor.
Btw, what will be the way to plug in an alternative hash function?
That is, there doesn't seem to be a separation of interface
and implementation in your patch (like with a template or a base-class
you inherit from).
Richard.
>> > > + /* 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