[review] Precompute hash value for symbol_set_names

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307
......................................................................


Patch Set 1: Code-Review+1

(4 comments)

This looks good.  I found a few nits, see below.
I'll probably put in the threading patches "soon", they've
been few a through rounds of review and have been sitting for
a while now.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/minsyms.c 
File gdb/minsyms.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/minsyms.c@1252 
PS1, Line 1252: 
1243 | 	  else
1244 | 	    *copyto++ = *copyfrom++;
1245 | 	}
1246 |       *copyto++ = *copyfrom++;
1247 |       mcount = copyto - msymbol;
1248 |     }
1249 |   return (mcount);
1250 | }
1251 | 
1252 | struct computed_hash_values {

{ on next line.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/minsyms.c@1392 
PS1, Line 1392: 
1370 | #endif
     | ...
1383 | 		   /* This will be freed later, by symbol_set_names.  */
1384 | 		   char *demangled_name
1385 | 		     = symbol_find_demangled_name (msym, msym->name);
1386 | 		   symbol_set_demangled_name
1387 | 		     (msym, demangled_name,
1388 | 		      &m_objfile->per_bfd->storage_obstack);
1389 | 		   msym->name_set = 1;
1390 | 
1391 | 		   size_t idx = msym - msymbols;
1392 | 		   hash_values[idx].mangled_name_hash = htab_hash_string (msym->name);

over-long line?  Also, fast_hash


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/symtab.h 
File gdb/symtab.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/symtab.h@518 
PS1, Line 518: 
509 |    it.  Used for constructs which do not have a mangled name,
510 |    e.g. struct tags.  Unlike SYMBOL_SET_NAMES, linkage_name must
511 |    be terminated and either already on the objfile's obstack or
512 |    permanently allocated.  */
513 | #define SYMBOL_SET_LINKAGE_NAME(symbol,linkage_name) \
514 |   (symbol)->ginfo.name = (linkage_name)
515 | 
516 | /* Set the linkage and natural names of a symbol, by demangling
517 |    the linkage name.  Optionally, HASH can be set to the value
518 |    of htab_hash_string (linkage_name) (if nullterminated), to

This has to refer to `fast_hash` now.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/symtab.h@526 
PS1, Line 526: 
517 |    the linkage name.  Optionally, HASH can be set to the value
518 |    of htab_hash_string (linkage_name) (if nullterminated), to
519 |    speed up this function.  */
520 | #define SYMBOL_SET_NAMES(symbol,linkage_name,len,copy_name,objfile)	\
521 |   symbol_set_names (&(symbol)->ginfo, linkage_name, len, copy_name, \
522 | 		    (objfile)->per_bfd)
523 | extern void symbol_set_names (struct general_symbol_info *symbol,
524 | 			      const char *linkage_name, int len, bool copy_name,
525 | 			      struct objfile_per_bfd_storage *per_bfd,
526 | 			      hashval_t hash = 0);

I suppose an optional<hashval_t> would avoid the problem
when the hash value is actually 0



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
Gerrit-Change-Number: 307
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 29 Oct 2019 19:28:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment