On 07/22/13 14:55, Keith Seitz wrote:
> On 07/22/2013 01:06 PM, Michael Eager wrote:
>> On 07/22/13 10:48, Keith Seitz wrote:
>>> On 07/17/2013 11:51 AM, Michael Eager wrote:
>>
>>>>   void
>>>> +annotate_linkage_name (void)
>>>> +{
>>>> +  if (annotation_level == 2)
>>>> +    printf_filtered (("\n\032\032linkage_name\n"));
>>>> +}
>>>> +
>>>
>>> This is still missing a (trivial) comment. [IIRC, we require comments
>>> for *all* functions, even if
>>> they are pretty trivial.]
>>
>> The Changelog contains
>>        * annotate.c (annotate_linkage_name): New.
>>        * annotate.h (annotate_linkage_name): New decl.
>>
>> Is something else needed?
>
> Yes, all functions need a comment, e.g.,
>
> /* Emit an annotation for a symbol's linkage name.  */
>
> void
> annotate_linkage_name (void)
> {
>     /* ... */
> }

This is pretty pointless busywork, serving no useful purpose.

There are 66 annotate_<whatever> functions in annotate.c.  Only
one function has a comment.  This comment actually provides
additional information which would not simply repeat the function name.

I added a comment.  Now there are two commented annotate functions,
one of which is meaningful.


>> diff --git a/gdb/testsuite/gdb.cp/display-linkage-name.exp

>> +
>> +gdb_test_no_output "set display-linkage-name off" ""
>
> You can omit the trailing quotes. gdb_test_no_output will then use the command name. Fortunately,
> this is only used once so the test's name is unique.

Done.

>> +gdb_test_no_output "set display-linkage-name on" ""
>
> Likewise.

Done.

>
> NOTE 2: These two tests output the same test name. You should supply the (optional) third parameter
> to gdb_test (MESSAGE) to uniquely identify this test. [You could simply use "show
> display-linkage-name-len 20" and "show display-linkage-name-len 10".]
>
> For future reference, you can use:
>
> $ make check RUNTESTFLAGS="my-test.exp"
> $ cat testsuite/gdb.sum | grep "PASS" | sort | uniq -c | sort -n

Or
   $ cat testsuite/gdb.sum | grep "PASS" | sort | uniq -d

>> +static void
>> +show_display_linkage_name (struct ui_file *file, int from_tty,
>> +          struct cmd_list_element *c, const char *value)
>> +{
>> +  fprintf_filtered (file, _("\
>> +Whether to display linkage name (symbol used by linker) of functions is %s.\n"),
>> +            value);
>> +}
>> +static void
>> +show_display_linkage_name_len (struct ui_file *file, int from_tty,
>> +          struct cmd_list_element *c, const char *value)
>> +{
>> +  fprintf_filtered (file, _("\
>> +Length of linkage names (symbol used by linker) to be displayed is %s.\n"),
>> +            value);
>> +}
>
> As silly as it seems, these two new functions need a comment.

Done.  Silly is perhaps a nicer term than I would use.

> After you fix those trivial things, I don't think there is anything more for me to review. You've
> already got documentation approval from Eli, so all that's left is a global maintainer's stamp of
> approval.

Thanks for your help.

Revised (hopefully final) patch attached.


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077