Re: [PATCH v2 3/5] gdb: update the docs for add_cmd and do_add_cmd to match reality

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-03-08 5:52 p.m., Simon Marchi via Gdb-patches wrote:> Just a few nits noted below.
> 
> Another good cleanup if you feel like it would be to move all the
> declarations of the functions defined in cli/cli-decode.c from command.h
> to cli/cli-decode.h.  We want to standardize that a declaration in foo.h
> has its definition in foo.c.
> 
>> diff --git a/gdb/command.h b/gdb/command.h
>> index 827a19637a2..df40cbf7119 100644
>> --- a/gdb/command.h
>> +++ b/gdb/command.h
>> @@ -155,18 +155,44 @@ extern bool valid_user_defined_cmd_name_p (const char *name);
>>  
>>  extern bool valid_cmd_char_p (int c);
>>  
>> -/* Const-correct variant of the above.  */
>> +/* Add a command named NAME in command list *LIST.
>>  
>> -extern struct cmd_list_element *add_cmd (const char *, enum command_class,
>> +   NAME and DOC are not duplicated. If they are not static string, they
> 
> Two spaces after period.
> 
>> +   must have been allocated with xmalloc or xstrdup and the
>> +   NAME_ALLOCATED/DOC_ALLOCATED fields must be set to 1 on the returned
>> +   command.
>> +
>> +   THECLASS is the top level category into which commands are broken down
>> +   for "help" purposes.
>> +
>> +   FUN should be the function to execute the command; it will get two
> 
> I'd say "is the" instead of "should be".  "should be" makes it sound
> like it's a suggestion but it could be something else, which is not the
> case.
> 
>> +   arguments, a character string (with leading and trailing blanks already
>> +   eliminated) containing the command arguments, and an integer indicating
>> +   whether input comes from a TTY or not.
> 
> I think this detailed doc about the callback's parameters would be
> better placed in the doc comment of the cmd_const_cfunc_ftype typedef.
> In that doc, you could refer to the parameters using their names.
> 
>> +
>> +   DOC is a documentation string for the command.
>> +   Its first line should be a complete sentence.
>> +   It should start with ? for a command that is an abbreviation
>> +   or with * for a command that most users don't need to know about.
>> +
>> +   If NAME already existed in *LIST, all its hooks and aliases are moved
>> +   to the new command.
>> +
>> +   Return a pointer to the added command (not necessarily the head of
>> +   *LIST).  */
> 
> I really like the way this doc is structured, one paragraph per
> parameter with some space in between, very legible.
> 
> Simon
> 

Note that if you send a new version of just that patch, we can approve
merge it on its own.

Simon