This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch 6/9] Mechanical symtab->filename -> symtab_to_filename
On Mon, 21 Jan 2013 20:33:47 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
>
> Jan> Hi,
> Jan> this patch is mostly a mechanical substitution:
> Jan> symtab-> filename -> symtab_to_filename (symtab)
>
> Jan> - && loc->line_number == sal.line)
> Jan> - {
> Jan> - if (compare_filenames_for_search (loc->symtab->filename,
> Jan> - sal.symtab->filename))
> Jan> - line_match = 1;
> Jan> - }
> Jan> + && loc->line_number == sal.line
> Jan> + && filename_cmp (symtab_to_fullname (loc->symtab),
> Jan> + sal_fullname) == 0)
> Jan> + line_match = 1;
>
> I found the patch a bit difficult because some spots are the mechanical
> conversion you mention, but others, like this, use symtab_to_fullname.
I am aware of it and I appreciate your reading it. Maybe I could split this
patch to more-mechanical and less-mechanical but I did not.
> Jan> + if (function_sal->symtab != NULL
> Jan> + && compare_filenames_for_search (function_sal->symtab->filename,
> Jan> + e->filename))
> Jan> + return 1;
>
> This spot still uses symtab->filename directly.
> Is that intentional? If so I think it should perhaps have a comment.
Yes.
+ /* Check first sole SYMTAB->FILENAME. It does not need to be
+ a substring of symtab_to_fullname as it may contain "./" etc. */
> Jan> + if (symname == NULL
> Jan> + || filename_cmp (symname, symtab_to_filename (s)) == 0)
>
> This spot uses filename_cmp with symtab_to_filename.
> Is that intentional?
> It seems a bit strange to me to allow the filename-display setting to
> control comparisons.
Because this is a maintenance command.
It has already contained filename_cmp and not compare_filenames_for_search.
I am not completely sure with it but for maintenance commands I have tried to
follow:
* Keep backward compatibility of behavior as long as is set the (currently
default) setting "set filename-display relative".
* Make it always working for copy-pasted filenames.
* Do not make the comparisons too relaxed, while false ambiguous-location
breakpoint set does not hurt much for maintenance commands I believe GDB
should display only exactly what it was asked for, nothing else.
> Jan> if (! regexp
> Jan> - || re_exec (symtab->filename))
> Jan> + || re_exec (symtab_to_filename (symtab)))
>
> Likewise.
Likewise, a maintenance command.
> Jan> @@ -150,7 +152,7 @@ tui_set_source_content (struct symtab *s,
> [...]
> Jan> - s->filename) == 0
> Jan> + symtab_to_filename (s)) == 0
>
> Likewise.
This was a larger change, it will be a new patch part. This filename was only
internally stored in TUI and it should be symtab_to_fullname.
Thanks,
Jan