Re: [PATCH] Add proper handling for non-local references in nested functions

[Pierre-Marie de Rodat <derodat@adacore.com>]

Pedro,

On 05/29/2015 02:28 PM, Pedro Alves wrote:
>> This would look cleaner indeed. It's a big change itself though so if
>> most consider this as a good idea I don't mind doing it... although it
>> would be for another commit!
>
> I would think it great if someone did that.  :-)

Okay... I may give it a try, then. ;-)

> Thanks for the update, and sorry about the delay.

No problem. I probably won't be able to resume my work on this until 
July, so more delay is to be expected. :-/ The good news is that then, 
my GCC patches to fix the corresponding DWARF may be integrated. :-)

> This overall looks very reasonable to me.  It's fine with me to
> let the core changes in, and address Python API issues separately.

Understood, thanks.

> Add:
>
> /* See block.h.  */

I will.

> It'd be great if you could skim over the patch add any missing
> function intro comments.  You've already done a good job at that,
> I think only here and there missed it.

Will double-check and fix.

>> +       VAR_BLOCK is needed there's a possibility for VAR to be outside FRAME.
>
> I think an "if" is missing after "needed".

Absolutely.

>> -      val = read_var_value (var, frame);
>> +      /* READ_VAR_VALUE needs a block in order to deal with non-local
>> +	 references (i.e. to handlee nested functions). In this context, we
>
> typo "handle".  Double space after period.

Will fix.

>> +# Please email any bugs, comments, and/or additions to this file to:
>> +# bug-gdb@gnu.org
>
> It no longer makes sense to add this email address to tests.  Please
> drop it (here and elsewhere).

Sure.

>> +void iter_str (const char *str, void (*callback) (char c))
>
> Could you make this follow GNU formatting?  That is:
>
> void
> iter_str (const char *str, void (*callback) (char c))
>
> Here and elsewhere.

I will.

>> +
>> +# Check we get correct values for both local and non-local variable references.
>> +
>> +# Note that in order to get the following test passing, one has to use a
>> +# patched GCC: see <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927>.
>> +gdb_test "print first"        "1"
>> +gdb_test "print parent_first" "1"
>
> Please make this XFAIL instead of FAIL with unpatched GCC.
>
> Otherwise looks good to me.

Ok. Thank you again for the review!

-- 
Pierre-Marie de Rodat