Re: [PATCH v3 3/5] Introduce gdb_tilde_expand

[Pedro Alves <palves@redhat.com>]

On 09/21/2017 11:59 PM, Sergio Durigan Junior wrote:

> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index cbafb13837..507fdc4120 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -54,6 +54,8 @@
>  #include "tui/tui.h"	/* For tui_active et.al.  */
>  #endif
>  
> +#include "gdb_tilde_expand.h"
> +
>  #include <fcntl.h>
>  #include <algorithm>
>  #include <string>
> @@ -410,10 +412,8 @@ cd_command (char *dir, int from_tty)
>       repeat might be useful but is more likely to be a mistake.  */
>    dont_repeat ();
>  
> -  gdb::unique_xmalloc_ptr<char> dir_holder
> -    (tilde_expand (dir != NULL ? dir : "~"));
> -  dir = dir_holder.get ();
> -
> +  std::string expanded_path = gdb_tilde_expand (dir != NULL ? dir : "~");
> +  dir = (char *) expanded_path.c_str ();
>    if (chdir (dir) < 0)
>      perror_with_name (dir);
>  
> @@ -438,7 +438,7 @@ cd_command (char *dir, int from_tty)
>  	len--;
>      }
>  
> -  dir_holder.reset (savestring (dir, len));
> +  gdb::unique_xmalloc_ptr<char> dir_holder (savestring (dir, len));
>    if (IS_ABSOLUTE_PATH (dir_holder.get ()))
>      {
>        xfree (current_directory);


I realized something: in light of the fact that "cd" is not what
is used to specify the inferior's cwd anymore since v1, patching
this particular use of tilde_expand, and not others seems arbitrary.

I.e., this now looks like kind of a spurious change to me, and
I think you should drop the changes to this file...

> +/* See common/gdb_tilde_expand.h.  */
> +
> +std::string
> +gdb_tilde_expand (const char *dir)
> +{
> +  gdb_glob glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR, NULL);

By my reading of man glob, GLOB_TILDE_CHECK already implies GLOB_TILDE.

I'm not sure whether GLOB_ONLYDIR is the right thing to do in
a function whose name doesn't suggest that it concern itself
with anything other than tilde expansion.  E.g., if we replaced
tilde_expand with gdb_tilde_expand in place that expect to match
files (like e.g., gdb_print_filename), then would this work as is?

Maybe you should rename this to gdb_tilde_expand_dir (leaving
the file names as is).

Thanks,
Pedro Alves