Re: [rfa] symbol hashing, part 1/n - updates to hash functions

[Elena Zannoni <ezannoni@cygnus.com>]

Daniel Jacobowitz writes:
 > On Thu, Oct 11, 2001 at 07:58:20PM -0400, Elena Zannoni wrote:
 > > Daniel Jacobowitz writes:
 > >  > This patch still has two logical parts; if you strongly prefer I can break
 > >  > it up further, but they are somewhat intertwined and I think neither should
 > >  > be objectionable.  They are:
 > >  >   - Fix a looping bug in msymbol_hash_iw.  It would not stop on '(' if there
 > >  > was whitespace before it.
 > >  >   - Update to use the identifier hash function that libiberty uses, and
 > >  > more buckets.
 > >  > 
 > >  > Is this OK?
 > > 
 > > Looks ok to me in theory. Except that, why was the
 > > 
 > >  '% MINIMAL_SYMBOL_HASH_SIZE;'
 > > 
 > > bit moved outside of the msymbol_hash and msymbol_hash_iw functions?
 > > You still do the same operation with the results returned by the two
 > > functions anyway. 
 > > 
 > > Also, where are these 2 functions used besides mynsyms.c?  I think we
 > > should make them static and remove the extern from symtab.h.
 > 
 > Both the moving of modulus and the no-other-uses are addressed by the
 > hashing patches.  These are the hash functions I will use on the
 > symtabs; they work for symbols as well as for minsyms.  A symtab has a
 > dynamic number of buckets.
 > 

Ok, I can see what's coming. But then, I would definitely prefer to
move the '%' out of the functions only when the rest of the patch is
submitted. It doesn't really fit with the changes you are making in
this patch.

 > > Can you give me an example where the '(' error comes up? (Just so I
 > > understand it better).  How did you come up with the number of
 > > buckets? Is this also used in libiberty?
 > 
 > The '(' error looks like this:
 > 
 > Hash the string "operator* ()".
 > At one point, string = " ()".  The initial whitespace loop changes this
 > to "()".  Then the character is not hashed (because of the if test
 > already present), but ++string is triggered.  The while loop now
 > continues, because *string == ')' instead of '('.
 > 

OK, thanks.

 > The number of blocks I just came up with by experimentation (well, Dan
 > did, and then I experimented with it and was satisfied).  Libiberty
 > uses expandable hash tables; I could simply use them instead, but I'd
 > rather postpone that change until we've got the rest of hashing in
 > place.
 > 

Yes, ok.

 > > Can you fix it and resubmit?
 > 
 > After my explanations, does anything else need fixing?
 > 

Just hold on the '%' move until the next patch.
Everything else is fine.

 > Thanks for looking at these patches!
 > 

Thanks for bearing with me!

Elena


 > -- 
 > Daniel Jacobowitz                           Carnegie Mellon University
 > MontaVista Software                         Debian GNU/Linux Developer