Re: [PATCH 3/4] Introduce gdb_chdir

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Wednesday, September 13 2017, Pedro Alves wrote:

> On 09/12/2017 05:23 AM, Sergio Durigan Junior wrote:
>
>> +#include "common-defs.h"
>> +#include <glob.h>
>> +#include "gdb_chdir.h"
>
> As per the guidelines, the module's own header should be
> included first thing  right after "common-defs.h".  That
> allows exposing unsatisfied dependencies (missing includes)
> in the header, if any is missing.

Done.

>> +/* Perform path expansion (i.e., tilde expansion) on DIR, and return
>> +   the full path.  */
>> +
>> +static std::string
>> +expand_path (const char *dir)
>
> Since this is particularly about tilde expansion,
> and a replacement for "tilde_expand", did you consider calling
> it gdb_tilde_expand and using it throughout?  If this were an
> extern function, I'd press for having "tilde" in its name,
> to make the call sites a bit more obvious.

Sure, no problem in renaming it.  Just to clarify: when you mean "use it
throughout", are saying that this should be used to replace readline's
"tilde_expand" elsewhere on GDB?

>> +{
>> +  glob_t g;
>> +  std::string expanded_dir;
>
> Move declaration further below to the initialization line,
> to avoid pointlessly default-constructing the string:
>
>   std::string expanded_dir = g.gl_pathv[0];

Done.

>> +  int err = glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR,
> NULL, &g);
>> +
>> +  if (err != 0)
>> +    {
>> +      if (err == GLOB_NOMATCH)
>> +	error (_("No such directory '%s'.  Failure to set cwd."), dir);
>
> The "Failure to set cwd" string doesn't seem like an error that
> should be in "expand_path"?  OK, not really an issue since this
> is a single-use static function...

You're right.  I'll make sure to display this error on "gdb_chdir"
instead.

>> +
>> +      error (_("Could not process directory '%s'."), dir);
>> +    }
>> +
>> +  gdb_assert (g.gl_pathc > 0);
>> +  /* "glob" may return more than one match to the path provided by the
>> +     user, but we are only interested in the first match.  */
>> +  expanded_dir = g.gl_pathv[0];
>> +  globfree (&g);
>
> Pedantically, this isn't strictly exception safe, since
> either the gdb_assert or the expanded_dir initialization
> (a string dup which can fail with out of memory) could throw.
>
> I think I'd write a thin RAII wrapper around glob that'd have a
> single "glob_t m_g;" field, that'd call glob in the ctor, and
> globfree in the dtor.

OK, I can do that, no problem.

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/