Daniel Jacobowitz wrote:
>> 	* bcache.c (bcache_data): Call bcache_added function.
>> 	(bcache_added): New function name. Body of function bcache_data
>> 	is used here with the addition of 'added' argument. The argument
>> 	is optional and if specified, bcache_added returns 1 if the data
>> 	was added and 0 if it was found in the hash.
> 
> Whenever you find yourself writing this sort of explanation in the
> changelog, it's a sign that you should have a shorter changelog entry
> and a long comment in the source code.  Please move the explanation to
> bcache.h.

Comments added.

>> +const void *
>> +bcache (const void *addr, int length, struct bcache *bcache)
>> +{
>> +  return bcache_data (addr, length, bcache);
>> +}
>> +
>> +void *
>> +bcache_added (const void *addr, int length, struct bcache *bcache, 
>> +		   int *added)
> 
> It should return a const pointer, like bcache.  Also the indentation
> on the second line is too deep.

bcache_added is more like bcache_data which returns void*. It would make sense to return void const* 
but then I would have to change const-ness in many places (too many: I would leave that for some 
other patch).

> 
>>      case DW_TAG_namespace:
>> -      add_psymbol_to_list (actual_name, strlen (actual_name),
>> +      add_psymbol_to_global_list (actual_name, strlen (actual_name),
>>  			   VAR_DOMAIN, LOC_TYPEDEF,
>> -			   &objfile->global_psymbols,
>>  			   0, (CORE_ADDR) 0, cu->language, objfile);
> 
> Please adjust indentation on the subsequent lines, since the open
> paren moved over.

Done.

> 
>>      case DW_TAG_enumerator:
>> -j      add_psymbol_to_list (actual_name, strlen (actual_name),
>> +      if (cu->language == language_cplus
>> +	  || cu->language == language_java)
>> +	add_psymbol_to_global_list (actual_name, strlen (actual_name),
>> +				    VAR_DOMAIN, LOC_CONST,
>> +				    0, (CORE_ADDR) 0, cu->language, objfile);
> 
> This part I don't understand.  Why does the behavior of enumerators
> depend on the language?  The logic for finding them during lookup is
> the same.

I must admit that I am confused a bit too. I followed the existing logic 
but now that you mentioned it, I realized I do not have a good explanation. 
I am inclined a bit to store them to the static list for C++, but for now 
I would just leave it as-is.

> 
> In fact, if lookup_partial_symbol will never find a global symbol
> with an identical name why do we need them for any duplicated symbol,
> even legitimately duplicated things like static functions?  

I don't think static functions are a good example since they would not 
go into the global list anyway (see dwarf2read.c, case DW_TAG_subprogram).

The attached is revised patch with the changes above; additionally, it 
calls new add_partial_symbol_to_global_list for all partial symbols that 
are being added to the global list. 


Thanks,
Aleksandar Ristovski
QNX Software Systems



ChangeLog:

	* bcache.c (bcache_data): Call bcache_added function.
	(bcache_added): New function name. Body of function bcache_data
	is used here with the addition of 'added' argument. 
	* bcache.h (bcache_added): New function.
	* dwarf2read.c (add_partial_symbol): Make call to 
	new function add_psymbol_to_global_list for partial symbols that
	are being added to the global list.
	(load_partial_dies): Likewise.
	* symfile.c (add_psymbol_to_bcache): New helper function, takes part of
	work from add_psymbol_to_list - initialises partial symbol and stashes
	it in objfile's cache.
	(append_psymbol_to_list): New helper function, takes other part of 
	work from add_psymbol_to_list - adds partial symbol to the given list.
	(add_psymbol_to_list): Call helper functions instead of doing work 
	here. Functionally, the function hasn't changed.
	(add_psymbol_to_global_list): New function, adds partial symbol to
	global list and does it only once per objfile.
	* symfile.h (add_psymbol_to_global_list): New function.