[libcamera-devel] [PATCH v3 1/6] libcamera: utils: Add an internal dirname helper

[Laurent Pinchart]

Hi Kieran,

On Fri, Feb 21, 2020 at 11:15:53AM +0000, Kieran Bingham wrote:
> On 20/02/2020 20:24, Laurent Pinchart wrote:
> > On Thu, Feb 20, 2020 at 04:56:59PM +0000, Kieran Bingham wrote:
> >> Provide a wrapped dirname call which returns a std::string.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  src/libcamera/include/utils.h |  1 +
> >>  src/libcamera/utils.cpp       | 17 +++++++++++++++++
> >>  2 files changed, 18 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..3fd3aeaf822a 100644
> >> --- a/src/libcamera/utils.cpp
> >> +++ b/src/libcamera/utils.cpp
> >> @@ -70,6 +70,23 @@ char *secure_getenv(const char *name)
> >>  #endif
> >>  }
> >>  
> >> +/**
> >> + * \brief identify the dirname portion of a path
> > 
> > s/identify/Identify/
> > 
> >> + * \param[in] path The full path to parse
> >> + *
> > 
> > Should we add that this function conforms to the behaviour of the
> > dirname() function defined by POSIX ?
> 
> No - because my version doesn't and didn't intend to :-)
> 
> >> + * \returns A string of the directory component of the path
> > 
> > s/returns/return/
> > 
> >> + */
> >> +std::string dirname(const std::string &path)
> >> +{
> >> +	size_t pos = path.rfind('/', path.length());
> >> +
> >> +	if (pos != std::string::npos) {
> >> +		return (path.substr(0, pos));
> >> +	}
> > 
> > No need for braces and parentheses around path.substr(0, pos).
> > 
> >> +
> >> +	return path;
> > 
> > To be compatible with the glibc implementation, you should return the
> > string "." if the path doesn't contain a '/'. Furthemore, if the whole
> > path string is equal to "/", then you should return "/", while the above
> > returns an empty string. There's also the case of trailing '/' in the
> > path that is not handled correctly.
> > 
> > How about the following (only compile-test) ?
> 
> Would you like to submit this as your own patch ?
> Or for me to take it verbatim ...

Feel free to take it, I have no issue with that. It just caught my eyes
that the implementation wasn't compliant with POSIX dirname(), and I
thought it could lead to confusion.

> > /**
> >  * \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();
> > 	while (pos > 0 && path[pos-1] == '/')
> > 		pos--;
> > 
> >         if (!pos)
> > 		return "/";
> > 
> > 	/*
> > 	 * Find the previous slash. If the path contains no non-trailing slash,
> > 	 * return ".".
> > 	 */
> > 	while (pos > 0 && path[pos-1] != '/')
> > 		pos--;
> > 
> > 	if (!pos)
> > 		return ".";
> > 
> > 	/*
> > 	 * Return the directory name until (but not including) the slash. If
> > 	 * this would result in an empty string, return "/".
> > 	 */
> > 	if (pos == 1)
> > 		return "/";
> > 
> > 	return path.substr(0, pos - 1);
> > }
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> That may be a bit redundant if it's on /your/ code. ...

Only the implementation is mine :-)

> >> +}
> >> +
> >>  /**
> >>   * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1,
> >>   *				     InputIt2 first2, InputIt2 last2)

-- 
Regards,

Laurent Pinchart