Re: [PATCH v3 3/5] Introduce gdb_tilde_expand

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Friday, September 22 2017, Pedro Alves wrote:

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

Yeah, you're right.  I still intend to keep the cleanups, if that's OK
for you.

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

Yes, but I think it pays to be explicit in this case.

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

Or I could simply remove GLOB_ONLYDIR (which is a leftover from the
previous versions of the patch) and make this function generic.  In
fact, I think I prefer this approach, because according to the manpage:

GLOB_ONLYDIR
	This is a hint to  glob() that the caller is interested
	only  in directories  that match  the pattern.   If the
	implementation    can   easily    determine   file-type
	information, then  nondirectory files are  not returned
	to the  caller.  However,  the caller must  still check
	that returned  files are directories.  (The  purpose of
	this flag  is merely  to optimize performance  when the
	caller is interested only in directories.)

I.e., it's only a helper flag for "glob".

I'll remove GLOB_ONLYDIR and resubmit the patch.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/