This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 3/5] Introduce gdb_tilde_expand
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/