| Message ID | 20200221163130.4791-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@ideasonboard.com> > + return TestFail; > + > return TestPass; > } > };
Hi Laurent, On 22/02/2020 10:27, Laurent Pinchart wrote: > 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@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. Ah yes > >> + "", >> + "///", >> + "/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. Indeed, I wanted direct expected results hardcoded for validation. I also liked your utils::split test implementation, and I was tempted to make this a vector of struct { char * path, char * expected } , but I just went with this for speed, and because it's nice to show the whole results in the event of a failure. >> + >> + 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 ? Sure, Mostly this code came in useful while I was fixing the implementation so I /hope/ it will never be broken, and thus this code shouldn't ever print (unless someone extends the tss?) > >> + >> + 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) > > ? Yes, that would be better in case the TestPass/TestFail values ever gets changed underneath... > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks, fixups applied. > >> + return TestFail; >> + >> return TestPass; >> } >> }; >
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 = { + "", + "///", + "/bin", + "/usr/bin", + "//etc////", + "//tmp//d//", + "current_file", + "./current_file", + "./current_dir/", + "current_dir/", + }; + + std::vector<std::string> expected = { + ".", + "/", + "/", + "/usr", + "/", + "//tmp", + ".", + ".", + ".", + ".", + }; + + 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; + + cerr << "results: " << endl; + for (const auto &path : results) + cerr << " " << path << endl; + + return TestFail; + } + + return 0; + } + int run() { /* utils::hex() test. */ @@ -71,6 +121,10 @@ protected: return TestFail; } + /* utils::dirname() tests. */ + if (testDirname()) + return TestFail; + return TestPass; } };
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@ideasonboard.com> --- src/libcamera/include/utils.h | 1 + src/libcamera/utils.cpp | 48 +++++++++++++++++++++++++++++++ test/utils.cpp | 54 +++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+)