[libcamera-devel] [PATCH v4 1/6] libcamera: utils: Add a C++ dirname implementation

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Feb 21, 2020 at 04:31:25PM +0000, Kieran Bingham wrote:
> Provide a std::string based implementation which conforms to the
> behaviour of the dirname() fucntion defined by POSIX.
> 
> Tests are added to cover expected corner cases of the implementation.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/include/utils.h |  1 +
>  src/libcamera/utils.cpp       | 48 +++++++++++++++++++++++++++++++
>  test/utils.cpp                | 54 +++++++++++++++++++++++++++++++++++
>  3 files changed, 103 insertions(+)
> 
> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> index 080ea6614de0..940597760ee2 100644
> --- a/src/libcamera/include/utils.h
> +++ b/src/libcamera/include/utils.h
> @@ -33,6 +33,7 @@ namespace utils {
>  const char *basename(const char *path);
>  
>  char *secure_getenv(const char *name);
> +std::string dirname(const std::string &path);
>  
>  template<class InputIt1, class InputIt2>
>  unsigned int set_overlap(InputIt1 first1, InputIt1 last1,
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> index 453e3b3b5995..f566e88cec5b 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -70,6 +70,54 @@ char *secure_getenv(const char *name)
>  #endif
>  }
>  
> +/**
> + * \brief Identify the dirname portion of a path
> + * \param[in] path The full path to parse
> + *
> + * This function conforms with the behaviour of the %dirname() function as
> + * defined by POSIX.
> + *
> + * \return A string of the directory component of the path
> + */
> +std::string dirname(const std::string &path)
> +{
> +	if (path.empty())
> +		return ".";
> +
> +	/*
> +	 * Skip all trailing slashes. If the path is only made of slashes,
> +	 * return "/".
> +	 */
> +	size_t pos = path.size() - 1;
> +	while (path[pos] == '/') {
> +		if (!pos)
> +			return "/";
> +		pos--;
> +	}
> +
> +	/*
> +	 * Find the previous slash. If the path contains no non-trailing slash,
> +	 * return ".".
> +	 */
> +	while (path[pos] != '/') {
> +		if (!pos)
> +			return ".";
> +		pos--;
> +	}
> +
> +	/*
> +	 * Return the directory name up to (but not including) any trailing
> +	 * slash. If this would result in an empty string, return "/".
> +	 */
> +	while (path[pos] == '/') {
> +		if (!pos)
> +			return "/";
> +		pos--;
> +	}
> +
> +	return path.substr(0, pos + 1);
> +}
> +
>  /**
>   * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1,
>   *				     InputIt2 first2, InputIt2 last2)
> diff --git a/test/utils.cpp b/test/utils.cpp
> index db1fbdde847d..e4184e39ce32 100644
> --- a/test/utils.cpp
> +++ b/test/utils.cpp
> @@ -19,6 +19,56 @@ using namespace libcamera;
>  class UtilsTest : public Test
>  {
>  protected:
> +	int testDirname()
> +	{
> +		std::vector<std::string> paths = {

You can make this const.

> +			"",
> +			"///",
> +			"/bin",
> +			"/usr/bin",
> +			"//etc////",
> +			"//tmp//d//",
> +			"current_file",
> +			"./current_file",
> +			"./current_dir/",
> +			"current_dir/",
> +		};
> +
> +		std::vector<std::string> expected = {

And this too.

> +			".",
> +			"/",
> +			"/",
> +			"/usr",
> +			"/",
> +			"//tmp",
> +			".",
> +			".",
> +			".",
> +			".",
> +		};

I was going to propose using the libc-provided dirname() function to
check for correctness instead of hardcoding the results, but that would
create a dependency on the libc implementation conforming with POSIX,
which may not be guaranteed on non-glibc platforms. Let's thus keep it
as-is.

> +
> +		std::vector<std::string> results;
> +
> +		for (const auto &path : paths)
> +			results.push_back(utils::dirname(path));
> +
> +		if (results != expected) {
> +			cerr << "utils::dirname() tests failed" << endl;
> +
> +			cerr << "expected: " << endl;
> +			for (const auto &path : expected)
> +				cerr << " " << path << endl;

"\t" instead of " " for additional clarity ?

> +
> +			cerr << "results: " << endl;
> +			for (const auto &path : results)
> +				cerr << " " << path << endl;

Here too ?

> +
> +			return TestFail;
> +		}
> +
> +		return 0;

		return TestPass;

> +	}
> +
>  	int run()
>  	{
>  		/* utils::hex() test. */
> @@ -71,6 +121,10 @@ protected:
>  			return TestFail;
>  		}
>  
> +		/* utils::dirname() tests. */
> +		if (testDirname())

		if (testDirname() != TestPass)

?

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +			return TestFail;
> +
>  		return TestPass;
>  	}
>  };

-- 
Regards,

Laurent Pinchart