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

[Kieran Bingham]

Hi Laurent,

On 20/02/2020 20:24, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> 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 ...


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

--
Kieran



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

-- 
Regards
--
Kieran