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

[Joel Brobecker <brobecker at adacore dot com>]

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...

> +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.

> +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.  */

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.

-- 
Joel