Re: [RFA] set/unset/show substitute-path commands (take 2)

[Joel Brobecker <brobecker at adacore dot com>]

> >    /* Quick way out if we already know its full name */
> > +
> >    if (*fullname)
> >      {
> > +      {
> > +        /* The user may have requested that source paths be rewritten
> > +           according to substitution rules he provided.  If a substitution
> > +           rule applies to this path, then apply it.  */
> > +        char *rewritten_fullname = rewrite_source_path (*fullname);
> > +
> > +        if (rewritten_fullname != NULL)
> > +           {
> > +             xfree (*fullname);
> > +             *fullname = rewritten_fullname;
> > +           }
> > +      }
> > +
> >        result = open (*fullname, OPEN_MODE);
> >        if (result >= 0)
> >  	return result;
> 
> You don't need the extra { } here.

OK. I used them to reduce the scope of rewritten_fullname to where
this variable is actually used, but I can see that it's not very useful.

> > +/* If the last character of PATH is a directory separator, then strip it.  */
> > +
> > +static void
> > +strip_trailing_directory_separator (char *path)
> > +{
> > +  const int last = strlen (path) - 1;
> > +
> > +  if (last < 0)
> > +    return;  /* No stripping is needed if PATH is the empty string.  */
> > +
> > +  if (IS_DIR_SEPARATOR (path[last]))
> > +    path[last] = '\0';
> > +}
> 
> Is this going to get you in trouble if PATH is "/" ?  I can imagine
> that happening.

I do not think so. In terms of the function itself, no problem, this
is reduced to the empty string. In terms of where we do the matching,
the function will correctly match anything that starts with '/', and
nothing else. I also verified that it's possible to remove that
substitution rule using the emtpy string.

However, you might prefer us to adjust the UI a bit, and still keep
the '/' at the UI level. So add some tweaks to "show substitute-path"
and "unset substitute-path" that will handle '/' as well as ''.

So instead of having "show sub" print `' -> `/mnt', it will print
`/' -> `/mnt'. And "unset sub" will accept both '' and '/' as argument
to delete that rule. It seems hardly necessary, but I don't mind adding
this (and the corresponding tests in subst.exp).

> Casting function types is a good way to get in trouble.  In this case,
> there's an answer handy: make_cleanup_freeargv.

How conveninent, thanks! Adjusted accordingly.

        * source.c: #include gdb_assert.h.
        (substitute_path_rule): New struct.
        (substitute_path_rules): New static global variable.
        (substitute_path_rule_matches): New function.
        (get_substitute_path_rule): New function.
        (rewrite_source_path): New function.
        (find_and_open_source): Add source path rewriting support.
        (strip_trailing_directory_separator): New function.
        (find_substitute_path_rule): New function.
        (add_substitute_path_rule): New function.
        (delete_substitute_path_rule): New function.
        (show_substitute_path_command): New function.
        (unset_substitute_path_command): New function.
        (set_substitute_path_command): New function.
        (_initialize_source): Add new substitute-path commands.
        * Makefile.in (source.o): Add dependency on gdb_assert.h.

Does it look better? Tested on x86-linux.

-- 
Joel

Attachment: subst.diff
Description: Text document