Re: [PATCH 3/4] Introduce gdb_chdir

[Pedro Alves <palves at redhat dot com>]

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.

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

> +{
> +  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];


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

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

> +
> +  return expanded_dir;
> +}
> +