Re: [PATCH] gdb: set filename-display shortpath support

[Azat Khuzhin <a3at.mail@gmail.com>]

On Mon, Dec 9, 2013 at 12:32 PM, Joel Brobecker <brobecker@adacore.com> wrote:
> Hello,
>
> Thanks for the patch. I've only had a chance to scan the patch, so
> cannot comment deeply. But I can make a few comments...

Thanks for you review.
I resent new patch with coding style issues resolved. (V2)

>
>> +2013-12-00  Azat Khuzhin  <a3at.mail@gmail.com>
>> +
>> +     * source.h (symtab_to_shortpath): Add it.
>> +     * source.c (filename-display): Add shortpath display.
>> +     * source.c (symtab_to_filename_for_display): Use symtab_to_shortpath.
>
> You don't need to repeat the source filename. In your case, the last
> 2 lines of the CL become:
>
>         * source.c (filename_display): Add shortpath display.
>         (symtab_to_filename_for_display): Use symtab_to_shortpath.
>
> Note also the '-' incorrectly used instead of '_' in filename_display.
>
> One small request, also. It's customary to provide the ChangeLog
> as text, rather than make it a ChagneLog diff. If you take a look
> at a few random messages on this list, you'll probably understand.
> ChangeLog files are always getting new changes, and always at the
> same location, thus constantly generating sources of conflicts.
> Unless you have an automated conflict solver in your setup, it's
> fine to send the diff with the ChangeLog in the revision history,
> and then only add the entry just before pushing the change to
> our repository. Tom also posted some tips on how he was managing
> the ChangeLog entries (search for "ChangeLog management tip" in
> the gdb@sourceware.org mailing list archives, Oct 25th 2013).
>
> Overall, I don't remember if other commented, but this sounds like
> a useful idea.
>
>> +#undef MIN
>> +#define MIN(A, B) (((A) <= (B)) ? (A) : (B))
>
> You don't need that. Use "min", defined in defs.h if not already
> defined.

I searched by MIN (case-sensitive) that's why I didn't find it.
Thanks.

>
>> +const char *
>> +symtab_to_shortpath (struct symtab *symtab)
>
> Can you add a small comment explaining that the description of that
> function is in source.h? The usual form is something minimilistic
> such as:
>
> /* See source.h.  */

Forgot that. Thanks.

>
> It just allows us to quickly know that this function is expected to be
> documented elsewhere.
>
>> +  char *prev_slash_name = (char *)symtab->filename;
>> +  char *prev_slash_dir = (char *)symtab->dirname;
>> +  char *slash_name = (char *)symtab->filename;
>> +  char *slash_dir = (char *)symtab->dirname;
>> +  const char *shortpath = slash_name;
>
> Formatting nit: space after the ')'.
>
>> +  if (!slash_dir)
>> +    return shortpath;
>
> A recent Coding Style decision requires us to explicitly test against
> NULL -> if (slash_dir != NULL)
>
>> +
>> +  while ((slash_name = strstr(slash_name, SLASH_STRING)) &&
>> +         (slash_dir = strstr(slash_dir, SLASH_STRING)))
>
> Formatting nit: binary operators should be at the start of the next
> line, not at the end. Also, make sure to have a space before '('
> in function calls. There are other such violations in the rest of
> the code, which I will not repeat - can you fix the rest as well?
>
> But we also frown on assignments inside conditions. Can you rewrite
> the code to avoid that?
>> +    {
>> +      slash_name++;
>> +      slash_dir++;
>> +
>> +      if (strncmp(slash_name, slash_dir, MIN(slash_name - prev_slash_name, slash_dir - prev_slash_dir)))
>
> This should should be split to not exceed 80 chars (we like a soft limit
> of 70 characters, as long as practical).
>
>> +  basename  - display only basename of a filename\n\
>> +  relative  - display a filename relative to the compilation directory\n\
>> +  absolute  - display an absolute filename\n\
>> +  shortpath - display only non-common part of filename and compilation directory\n\
>
> This line is a little bit too long. It's a bit of  PIA to be breaking
> it; I think we should do it, for the sake of consistency, but it's not
> a strong opinion.

I think that there is no need in new line for breaking, so I just
break it in code.

>
> --
> Joel



-- 
Respectfully
Azat Khuzhin