Re: [patch 6/9] Mechanical symtab->filename -> symtab_to_filename

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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