Re: [PATCH] gdbserver: convert program_args to a single string

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simark@simark.ca> writes:

> On 2025-01-14 12:23, Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
>> 
>>>> /* Start a new process.
>>>> PROGRAM is the program name.
>>>> -   PROGRAM_ARGS is the vector containing the inferior's args.
>>>> +   PROGRAM_ARGS is a string containing all the inferior's arguments.
>>>> Returns the new PID on success, -1 on failure.  Registers the new
>>>> process with the process list.  */
>> 
>> Simon> I think this comment should just be removed, there's no point in
>> Simon> repeating the documentation from the base class.
>> 
>> FWIW I normally consider the "override" keyword to be sufficient
>> documentation in these cases; though there's also some code with a
>> comment pointing at the base class.
>
> Speaking of this, I don't really know what to do in these situations, I'd like
> if we could clarify what our style is.  Let's say you have:
>
> -- base.h
>
> struct base
> {
>   /* 1 */
>   virtual void method ();
> };
>
> -- base.c
>
> /* 2 */
>
> void base::method ()
> {
> }
>
> -- impl.c
>
> struct impl : public base
> {
>   /* 3 */
>   void method () override;
> };
>
> /* 4 */
>
> void impl::method ()
> {
> }
>
> Which comment should you have where?  We typically say "all declarations
> and definitions should have a comment", but I feel like it's unnecessary here.
>
>  - #1 is where you should have the documentation about the behavior of
>    `method`

As Tom said, the actual documentation could be at #1 or #2.  With C++
implementing more stuff in headers, having the docs at #1 makes more and
more sense I think.

>  - #2 would be the typical /* See base.h.  */, although I find these
>    comments a bit useless

It is a little useless, but I do like the consistency of all functions
having something.  And in some cases, when the declaration is not in the
obvious place, it can help find where to look.

>  - As you said, I think the `override` keyword is sufficient to make
>    comment #3 unnecessary, unless you want to specify something specific
>    to `impl`.  I don't see the need to point to the base class, that's
>    just implied by how the language works.

Agreed.

>  - I never know what to put for #4, you can't put /* See impl.c.  */,
>    since you're already in impl.c.  Again, I feel like a "See whatever"
>    comment is a bit useless: if you know C++, you know you need to go
>    look at the declaration of struct/class `impl`.

In cases where the base class declaration, and the out-of-line
definitions are in the same file I write /* See class declaration.  */
on the definitions (or something similar).

In this example I'd probably go with /* See base.h.  */ as that guides
folk to where the comment can be found.

Just thoughts.

Thanks,
Andrew