| Message ID | 20200220165704.23600-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > + * \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) ? /** * \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@ideasonboard.com> > +} > + > /** > * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1, > * InputIt2 first2, InputIt2 last2)
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@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@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) >
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@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@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)
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 + * \param[in] path The full path to parse + * + * \returns A string of the directory component of the path + */ +std::string dirname(const std::string &path) +{ + size_t pos = path.rfind('/', path.length()); + + if (pos != std::string::npos) { + return (path.substr(0, pos)); + } + + return path; +} + /** * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1, * InputIt2 first2, InputIt2 last2)
Provide a wrapped dirname call which returns a std::string. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/include/utils.h | 1 + src/libcamera/utils.cpp | 17 +++++++++++++++++ 2 files changed, 18 insertions(+)