Re: [PING 2][PATCH][PR gdb/19893] Fix handling of synthetic C++ references

[Martin Galvan <martin dot galvan at tallertechnologies dot com>]

On Wed, May 18, 2016 at 12:48 PM, Pedro Alves <palves@redhat.com> wrote:
> On 05/18/2016 03:47 PM, Martin Galvan wrote:
>
>>       * gdb.dwarf2/implref.exp: Rename to...
>>       * gdb.dwarf2/implref-const.exp: ...this.
>
> Were there changes other than renaming?  Please tweak
> your git config settings to detect renames / do "git diff -M".

Yeah, there were. Will add it to the changelog.

>> +static struct value *
>> +fetch_const_value_from_synthetic_pointer (sect_offset die, LONGEST byte_offset,
> Missing intro comment, which should leave no doubt on why is the function
> named "const value", but then returns a non-const value.

I meant it was a DW_AT_const_value, which (AFAIK) isn't necessarily
represented by a const struct value in gdb.

> But you can actually remove the "result" local and call "return" directly
> in both branches.

Yeah, I just like it when functions have a single return statement.
Will change it :)

>> +# This matches e.g. '{10, 1, 2, 3, 4}'
>> +set new_contents [format {\{%d[\d,\s]+\}} ${first_value}]
>
> Why not match exactly "{10, 1, 2, 3, 4}" ?

The idea was to let the test maintainer alter the array (e.g. add
more/different elements) whenever possible without having to touch the
TCL file as well. But if it's ok to assume it won't change then I can
simplify the regex a bit.

>> +      /* Else, we have a synthetic reference.  Don't print '@address'; we'll
>> +      show '<synthetic pointer>' instead through valprint_check_validity.
>> +      Notice however that 'set print object on' will still show '@address'.
>
> Is that a bug, or desirable?

I don't think it's a bug (the address that'll be shown is the
referenced variable's, not something like 0x0). If you ask me, I think
we can just leave it as a (documented) corner case.

Thanks for the feedback!