Re: [PATCH v3] kdb: kdb_support: Fix debugging information problem

[Doug Anderson]

Hi,

On Tue, Feb 2, 2021 at 3:15 AM Stephen Zhang <stephenzhangzsd@xxxxxxxxx> wrote:
>>
>>
>> > @@ -147,11 +141,10 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
>> >
>> >         if (symtab->mod_name == NULL)
>> >                 symtab->mod_name = "kernel";
>> > -       if (KDB_DEBUG(AR))
>> > -               kdb_printf("kdbnearsym: returns %d symtab->sym_start=0x%lx, "
>> > -                  "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
>> > -                  symtab->sym_start, symtab->mod_name, symtab->sym_name,
>> > -                  symtab->sym_name);
>> > +       kdb_dbg_printf(AR, "returns %d symtab->sym_start=0x%lx, "
>> > +               "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
>> > +               symtab->sym_start, symtab->mod_name, symtab->sym_name,
>> > +               symtab->sym_name);
>>
>> This indention doesn't line up, but it didn't before.  I guess I'd let
>> Daniel say if he likes this or would prefer different wrapping /
>> indentation.
>
>
> Thanks for your patience. You told me that  the alignment for continued arguments is to
> match up with the first argument, so I was confused that the first line and the second line
> here don't follow the rule.  There are many  cases like this in  this file.

There's a saying: all rules are made to be broken.

I think the general rule is that the 2nd line of arguments should line
up with the first.  However, sometimes that forces way too much
wrapping.  If it's "too ugly" to use the general rule, then you can
fall back to some alternate rule.  Usually this alternate rule is
something like indending all subsequent lines by one tab.  The
definition of "too ugly" is left to the judgement of the person
writing / reviewing the code.  In this case maybe the general rule
would make your call need to take up 3 lines?

If I had to make a judgement call on this code, I'd say:

1. It seems to have been written before the "don't split strings" rule
was in full force.  See
<https://www.kernel.org/doc/html/v5.10/process/coding-style.html#breaking-long-lines-and-strings>
where it says "never break user-visible strings such as printk
messages because that breaks the ability to grep for them".

2. If we fix #1, we're already blowing over the 80 columns limit for
this string anyway.

3. Once we blow over the 80 columns, might as well do it for the
second line.  So then you'd end up with:

<tab>kdb_dbg_printf(AR, "returns [...] (%s)\n",
<tab><tab>       ret, symtab->sym_start, [...], symtab->sym_name);