Hi Keith,

Thanks for your review.

On Thu, Mar 28, 2013 at 8:29 PM, Keith Seitz <keiths@redhat.com> wrote:
> On 03/25/2013 11:41 PM, Hui Zhu wrote:
>>
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -12963,6 +12963,15 @@ bkpt_re_set (struct breakpoint *b)
>>     breakpoint_re_set_default (b);
>>   }
>>
>> +static void
>> +dprintf_re_set (struct breakpoint *b)
>> +{
>
>
> This function needs a comment, even if it just mentions that this function
> is the re_set method for dprintf breakpoints.

Fixed.

>
>> +  breakpoint_re_set_default (b);
>> +
>> +  if (b->extra_string != NULL)
>> +    update_dprintf_command_list (b);
>> +}
>> +
>
>
> This will update the command list every time breakpoints are reset and could
> be limited to only those needing updating. Is there perhaps a reason to
> always do this?

I think it need, because it need to generate different commands with
different status for example:
      if (target_can_run_breakpoint_commands ())
	printf_line = xstrprintf ("agent-printf %s", dprintf_args);

>
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/dprintf-pending.c
>> @@ -0,0 +1,31 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2004-2013 Free Software Foundation, Inc.
>
>                 ^^^^^^^^^
> I think you meant only 2013. :-)

Fixed.

>
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/dprintf-pending.exp
>> +
>> +if [get_compiler_info] {
>> +    return -1
>> +}
>
>
> Can you please add braces around the expression here? [Yeah, that's a nit,
> but it is one of my Tcl pet peeves, and this is a new file. Time to stomp
> out the old crud.]

Fixed.

Post the new version according to your comments.

Best,
Hui

>
> Keith