This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 24 Jul 2014, at 03:48, Trevor Saunders <tsaunders@mozilla.com> wrote:

> On Thu, Jul 24, 2014 at 02:00:24AM +0200, Oleg Endo wrote:
>> On Wed, 2014-07-23 at 11:37 +0200, Richard Biener wrote:
>>> 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).
>> 
>> Isn't that what boost / std hash is actually doing?  Maybe something
>> like the attached example could be an improvement?
> 
> I don't really see why that makes it any easier to plug in a different
> hash algorithm

typedef incremental_hash<my_hash, my_hash_combiner> my_inc_hash;
                         ^^^^^^^
                      replace with a different class

True, it's not any 'easier' than replacing any other name in the code.
It makes it easier to mix different type dependent hash functions/
algorithms, though.  The way I understood it, Richard asked how to replace
the hash function for the incremental hash class.

> 
>> As with boost / std hash, the hash function is a template functor that
>> can be specialized on various types.  The 'incremental_hash' simply
>> plugs together a hash value combiner and a hash function.  Types that
>> are not implemented by the hash function (in this example) are caught at
>> compile time, since implicit conversions do not take place.  However, it
>> should also possible to write hash functions with a bunch of operator ()
>> overloads to utilize implicit conversions.
> 
> at that point it seems like its simpler and equivelent to just overload
> a global function.

AFAIR, there are some cases which are difficult to handle with overloads
(implicit conversions etc).  It could be a freestanding template
function that is specialized for the types in question.


> 
>> One advantage of this construct is that new types can be added along
>> with hash function specializations, without the need to touch any of the
>> existing hashing facilities.
>> 
>> I don't know how/whether this would fit with the already existing hash
>> map stuff (hash-table.h, hash-map.h).  It seems those don't support user
>> specified hash functions.
> 
> they do, see e.g. graphite-htab.h

Aha.  I see.


Cheers,
Oleg

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]