[PATCH] Replace the block_found global with explicit data-flow
Pierre-Marie de Rodat
derodat@adacore.com
Thu Jul 30 13:42:00 GMT 2015
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Replace-the-block_found-global-with-explicit-data-fl.patch
Type: text/x-diff
Size: 162023 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20150730/d46dd771/attachment.bin>
More information about the Gdb-patches
mailing list