Many thanks for your review, Doug!

On 07/25/2015 11:23 PM, Doug Evans wrote:
> There's a missing patch to scm-frame.c.:
>
> ../../bound-symbol/gdb/guile/scm-frame.c: In function ‘gdbscm_frame_read_var’:
> ../../bound-symbol/gdb/guile/scm-frame.c:914:8: error: incompatible types when assigning to type ‘struct symbol *’ from type ‘struct symbol_in_block’
>      var = lookup_symbol (var_name, block, VAR_DOMAIN, NULL);
>
> I've appended something that works here at the end.

I used to build with --disable-guile: I realize it is a bad idea to do 
this when submitting patches, so I have re-enabled it, fixed the errors 
(your changes are fine) and re-tested the patch.

> One high level note.
> What do you think of "block_symbol"?
> "symbol_in_block" reads a bit clumsy.

Agreed and done.

>> +	  struct symbol *sym;
>
> ====
> The name "sym" shadows "sym" defined at function entry.
> Renaming the one at function entry to "bsym" would be ok.

Indeed, done.

>> -	  convert_symbol_sym (context, identifier, sym, domain);
>> +	  convert_symbol_sym (context, identifier, sym.symbol, domain,
>> +			      sym.block);
>
> ====
> Just pass sym here, instead of sym.symbol and sym.block.

Done.

>> @@ -629,20 +634,19 @@ cp_lookup_symbol_imports_or_template (const char *scope,
>> [...]
>> -	      return result;
>> +	      return (struct symbol_in_block) {sym, NULL};
>
> ====
> Do we have to return NULL for block here?
> I haven't dug into this, but it seems like we could just return
> the "block" argument for the block here.

I think you are right, so I did it. However, since this lookup function 
does not always performs a symtab lookup, block_found was not always 
updated. So I guess callers currently disregard it and thus nothing will 
test if if it's the correct block returned. Anyway...

>> -		  return result;
>> +		  return (struct symbol_in_block) {sym, NULL};
>
> ====
> Do we have to return NULL for block here?
> I haven't dug into this, but it seems like we could just return
> the "block" argument for the block here.

For this one, I returned PARENT instead, since that's from where we are 
retrieving the template arguments. Same disclaimer about testing, though...

>> +          struct symbol_in_block sym
>> +	    = cp_lookup_symbol_imports_or_template (scope, name, block,
>> +						    domain);
>
> ====
> Coding conventions require a blank line here.
> I think a saw a few more places like this.
> [blank line after local variable definitions]

Added here, and on all sites which my patch affects and which I noticed. 
Thanks.

> LGTM with the rename and the following nits fixed.

Great, thanks again! Here's the updated patch, rebased on master and 
re-tested on x86_64-linux. I'll wait for tomorrow before pushing it in 
order give anyone a chance to object. :-)

-- 
Pierre-Marie de Rodat