[PATCH 3/4] Introduce gdb_chdir
Pedro Alves
palves@redhat.com
Wed Sep 13 16:07:00 GMT 2017
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;
> +}
> +
More information about the Gdb-patches
mailing list