Re: [PATCH v2 2/4] Add a new function child_path.

[Simon Marchi <simark at simark dot ca>]

On 2019-01-28 3:47 p.m., John Baldwin wrote:
> child_path returns a pointer to the first component in a child path
> that comes after a parent path.  This does not depend on trying to
> stat() the paths since they may describe remote paths but instead
> relies on filename parsing.  The function requires that the child path
> describe a filename that contains at least one component below the
> parent path and returns a pointer to the first component.
> 
> gdb/ChangeLog:
> 
> 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
> 	unittests/child-path-selftests.c.
> 	* common/pathstuff.c (child_path): New function.
> 	* common/pathstuff.h (child_path): New prototype.
> 	* unittests/child-path-selftests.c: New file.

Thanks, this LGTM.  Just minor comments below.

> ---
>  gdb/ChangeLog                        |  8 ++++
>  gdb/Makefile.in                      |  1 +
>  gdb/common/pathstuff.c               | 52 ++++++++++++++++++++++
>  gdb/common/pathstuff.h               |  6 +++
>  gdb/unittests/child-path-selftests.c | 64 ++++++++++++++++++++++++++++
>  5 files changed, 131 insertions(+)
>  create mode 100644 gdb/unittests/child-path-selftests.c
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 38d740e440..93a2cebe1c 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,11 @@
> +2019-01-28  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
> +	unittests/child-path-selftests.c.
> +	* common/pathstuff.c (child_path): New function.
> +	* common/pathstuff.h (child_path): New prototype.
> +	* unittests/child-path-selftests.c: New file.
> +
>  2019-01-28  John Baldwin  <jhb@FreeBSD.org>
>  
>  	* symfile.c (find_separate_debug_file): Look for separate debug
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 72ca855eb0..cec7ad32a4 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -411,6 +411,7 @@ SUBDIR_PYTHON_CFLAGS =
>  
>  SUBDIR_UNITTESTS_SRCS = \
>  	unittests/array-view-selftests.c \
> +	unittests/child-path-selftests.c \
>  	unittests/cli-utils-selftests.c \
>  	unittests/common-utils-selftests.c \
>  	unittests/copy_bitwise-selftests.c \
> diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
> index 11675303b3..d6beb8faf1 100644
> --- a/gdb/common/pathstuff.c
> +++ b/gdb/common/pathstuff.c
> @@ -147,6 +147,58 @@ gdb_abspath (const char *path)
>  
>  /* See common/pathstuff.h.  */
>  
> +const char *
> +child_path (const char *parent, const char *child)
> +{
> +  /* The child path must start with the parent path.  */
> +  size_t parent_len = strlen (parent);
> +  if (filename_ncmp (parent, child, parent_len) != 0)
> +    return NULL;
> +
> +  /* The parent path must be a directory and the child must contain at
> +     least one component underneath the parent.  */
> +  const char *child_component;
> +  if (IS_DIR_SEPARATOR (parent[parent_len - 1]))
> +    {
> +      /* The parent path ends in a directory separator, so it is a
> +	 directory.  The first child component starts after the common
> +	 prefix.  */
> +      child_component = child + parent_len;
> +    }
> +  else
> +    {
> +      /* The parent path does not end in a directory separator.  The
> +	 first character in the child after the common prefix must be
> +	 a directory separator.
> +
> +	 Note that CHILD must hold at least parent_len characters for
> +	 filename_ncmp to return zero.  If the character at parent_len
> +	 is nul due to CHILD containing the same path as PARENT, the
> +	 IS_DIR_SEPARATOR check will fail here.  */
> +      if (!IS_DIR_SEPARATOR (child[parent_len]))
> +	return NULL;
> +
> +      /* The first child component starts after the separator after the
> +	 common prefix.  */
> +      child_component = child + parent_len + 1;
> +    }
> +
> +  /* The child must contain at least one non-separator character after
> +     the parent.  */
> +  while (*child_component != '\0')
> +    {
> +      if (IS_DIR_SEPARATOR (*child_component))
> +	{
> +	  child_component++;
> +	  continue;
> +	}
> +      return child_component;

This might be simplified (avoid the continue) by reversing the logic, using
this as the loop body:

{
  if (!IS_DIR_SEPARATOR (*child_component))
    return child_component;

  child_component++;
}

> +    }
> +  return NULL;
> +}
> +
> +/* See common/pathstuff.h.  */
> +
>  bool
>  contains_dir_separator (const char *path)
>  {
> diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h
> index d43f337550..20a7bdda26 100644
> --- a/gdb/common/pathstuff.h
> +++ b/gdb/common/pathstuff.h
> @@ -48,6 +48,12 @@ extern gdb::unique_xmalloc_ptr<char>
>  
>  extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);
>  
> +/* If the path in CHILD is a child of the path in PARENT, return a
> +   pointer to the first component in the CHILD's pathname below the
> +   PARENT.  Otherwise, return NULL.  */
> +
> +extern const char *child_path (const char *parent, const char *child);
> +
>  /* Return whether PATH contains a directory separator character.  */
>  
>  extern bool contains_dir_separator (const char *path);
> diff --git a/gdb/unittests/child-path-selftests.c b/gdb/unittests/child-path-selftests.c
> new file mode 100644
> index 0000000000..2621eecef6
> --- /dev/null
> +++ b/gdb/unittests/child-path-selftests.c
> @@ -0,0 +1,64 @@
> +/* Self tests for child_path for GDB, the GNU debugger.
> +
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "defs.h"
> +#include "common/pathstuff.h"
> +#include "common/selftest.h"
> +
> +namespace selftests {
> +namespace child_path {
> +
> +/* Verify the result of a single child_path test.  */
> +
> +static bool
> +child_path_check (const char *parent, const char *child, const char *expected)
> +{
> +  const char *result = ::child_path (parent, child);
> +  if (result == NULL || expected == NULL)
> +    return result == expected;
> +  return strcmp (result, expected) == 0;
> +}
> +
> +/* Test child_path.  */
> +
> +static void
> +test ()
> +{
> +  SELF_CHECK (child_path_check ("/one", "/two", NULL));
> +  SELF_CHECK (child_path_check ("/one", "/one", NULL));
> +  SELF_CHECK (child_path_check ("/one", "/one/", NULL));
> +  SELF_CHECK (child_path_check ("/one", "/one//", NULL));
> +  SELF_CHECK (child_path_check ("/one", "/one/two", "two"));
> +  SELF_CHECK (child_path_check ("/one/", "/two", NULL));
> +  SELF_CHECK (child_path_check ("/one/", "/one", NULL));
> +  SELF_CHECK (child_path_check ("/one/", "/one/", NULL));
> +  SELF_CHECK (child_path_check ("/one/", "/one//", NULL));
> +  SELF_CHECK (child_path_check ("/one/", "/one/two", "two"));
> +}

Here are a few more edge cases I could think of:

  SELF_CHECK (child_path_check ("/one/", "/one//two", "two"));
  SELF_CHECK (child_path_check ("/one/", "/one//two/", "two/"));
  SELF_CHECK (child_path_check ("/one", "/onetwo", NULL));
  SELF_CHECK (child_path_check ("/one", "/onetwo/three", NULL));

> +
> +}
> +}
> +
> +void
> +_initialize_child_path_selftests ()
> +{
> +  selftests::register_test ("child_path",
> +			    selftests::child_path::test);
> +}
> +
> 

Simon