| Message ID | 20241220150759.709756-7-pobrn@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Fri, Dec 20, 2024 at 03:08:29PM +0000, Barnabás Pőcze wrote: > Just use an `std::vector` to store the arguments passed to > `InitGoogleTest()`. This removes the need for the map and > the separate `argc` variable used for size-keeping. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> Nice, that really seemed like a lot of not required complications Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > src/apps/lc-compliance/main.cpp | 36 +++++++++------------------------ > 1 file changed, 9 insertions(+), 27 deletions(-) > > diff --git a/src/apps/lc-compliance/main.cpp b/src/apps/lc-compliance/main.cpp > index cdd0bd515..3d9c51fc3 100644 > --- a/src/apps/lc-compliance/main.cpp > +++ b/src/apps/lc-compliance/main.cpp > @@ -80,45 +80,27 @@ static int initCamera(CameraManager *cm, OptionsParser::Options options) > > static int initGtestParameters(char *arg0, OptionsParser::Options options) > { > - const std::map<std::string, std::string> gtestFlags = { { "list", "--gtest_list_tests" }, > - { "filter", "--gtest_filter" } }; > - > - int argc = 0; > + std::vector<char *> argv; > std::string filterParam; > > - /* > - * +2 to have space for both the 0th argument that is needed but not > - * used and the null at the end. > - */ > - char **argv = new char *[(gtestFlags.size() + 2)]; > - if (!argv) > - return -ENOMEM; > - > - argv[0] = arg0; > - argc++; > + argv.push_back(arg0); > > - if (options.isSet(OptList)) { > - argv[argc] = const_cast<char *>(gtestFlags.at("list").c_str()); > - argc++; > - } > + if (options.isSet(OptList)) > + argv.push_back(const_cast<char *>("--gtest_list_tests")); > > if (options.isSet(OptFilter)) { > /* > * The filter flag needs to be passed as a single parameter, in > * the format --gtest_filter=filterStr > */ > - filterParam = gtestFlags.at("filter") + "=" + > - static_cast<const std::string &>(options[OptFilter]); > - > - argv[argc] = const_cast<char *>(filterParam.c_str()); > - argc++; > + filterParam = "--gtest_filter=" + options[OptFilter].toString(); > + argv.push_back(const_cast<char *>(filterParam.c_str())); > } > > - argv[argc] = nullptr; > - > - ::testing::InitGoogleTest(&argc, argv); > + argv.push_back(nullptr); > > - delete[] argv; > + int argc = argv.size(); > + ::testing::InitGoogleTest(&argc, argv.data()); > > return 0; > } > -- > 2.47.1 > >
On Fri, Dec 20, 2024 at 03:08:29PM +0000, Barnabás Pőcze wrote: > Just use an `std::vector` to store the arguments passed to > `InitGoogleTest()`. This removes the need for the map and > the separate `argc` variable used for size-keeping. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/apps/lc-compliance/main.cpp | 36 +++++++++------------------------ > 1 file changed, 9 insertions(+), 27 deletions(-) > > diff --git a/src/apps/lc-compliance/main.cpp b/src/apps/lc-compliance/main.cpp > index cdd0bd515..3d9c51fc3 100644 > --- a/src/apps/lc-compliance/main.cpp > +++ b/src/apps/lc-compliance/main.cpp > @@ -80,45 +80,27 @@ static int initCamera(CameraManager *cm, OptionsParser::Options options) > > static int initGtestParameters(char *arg0, OptionsParser::Options options) > { > - const std::map<std::string, std::string> gtestFlags = { { "list", "--gtest_list_tests" }, > - { "filter", "--gtest_filter" } }; > - > - int argc = 0; > + std::vector<char *> argv; > std::string filterParam; > > - /* > - * +2 to have space for both the 0th argument that is needed but not > - * used and the null at the end. > - */ > - char **argv = new char *[(gtestFlags.size() + 2)]; > - if (!argv) > - return -ENOMEM; > - > - argv[0] = arg0; > - argc++; > + argv.push_back(arg0); > > - if (options.isSet(OptList)) { > - argv[argc] = const_cast<char *>(gtestFlags.at("list").c_str()); > - argc++; > - } > + if (options.isSet(OptList)) > + argv.push_back(const_cast<char *>("--gtest_list_tests")); > > if (options.isSet(OptFilter)) { > /* > * The filter flag needs to be passed as a single parameter, in > * the format --gtest_filter=filterStr > */ > - filterParam = gtestFlags.at("filter") + "=" + > - static_cast<const std::string &>(options[OptFilter]); > - > - argv[argc] = const_cast<char *>(filterParam.c_str()); > - argc++; > + filterParam = "--gtest_filter=" + options[OptFilter].toString(); > + argv.push_back(const_cast<char *>(filterParam.c_str())); > } > > - argv[argc] = nullptr; > - > - ::testing::InitGoogleTest(&argc, argv); > + argv.push_back(nullptr); > > - delete[] argv; > + int argc = argv.size(); > + ::testing::InitGoogleTest(&argc, argv.data()); > > return 0; > } > -- > 2.47.1 > >
Hi Barnabás, Thank you for the patch. On Fri, Dec 20, 2024 at 03:08:29PM +0000, Barnabás Pőcze wrote: > Just use an `std::vector` to store the arguments passed to > `InitGoogleTest()`. This removes the need for the map and > the separate `argc` variable used for size-keeping. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/apps/lc-compliance/main.cpp | 36 +++++++++------------------------ > 1 file changed, 9 insertions(+), 27 deletions(-) > > diff --git a/src/apps/lc-compliance/main.cpp b/src/apps/lc-compliance/main.cpp > index cdd0bd515..3d9c51fc3 100644 > --- a/src/apps/lc-compliance/main.cpp > +++ b/src/apps/lc-compliance/main.cpp > @@ -80,45 +80,27 @@ static int initCamera(CameraManager *cm, OptionsParser::Options options) > > static int initGtestParameters(char *arg0, OptionsParser::Options options) > { > - const std::map<std::string, std::string> gtestFlags = { { "list", "--gtest_list_tests" }, > - { "filter", "--gtest_filter" } }; > - > - int argc = 0; > + std::vector<char *> argv; > std::string filterParam; > > - /* > - * +2 to have space for both the 0th argument that is needed but not > - * used and the null at the end. > - */ > - char **argv = new char *[(gtestFlags.size() + 2)]; > - if (!argv) > - return -ENOMEM; > - > - argv[0] = arg0; > - argc++; > + argv.push_back(arg0); > > - if (options.isSet(OptList)) { > - argv[argc] = const_cast<char *>(gtestFlags.at("list").c_str()); > - argc++; > - } > + if (options.isSet(OptList)) > + argv.push_back(const_cast<char *>("--gtest_list_tests")); Functions that take a char ** when they never modify the contents of the strings are annoying :-/ Would it be better to drop the const_char here and below, and add a single one when calling ::testing::InitGoogleTest() with ::testing::InitGoogleTest(&argc, const_cast<char **>(argv.data())); ? Either way, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > if (options.isSet(OptFilter)) { > /* > * The filter flag needs to be passed as a single parameter, in > * the format --gtest_filter=filterStr > */ > - filterParam = gtestFlags.at("filter") + "=" + > - static_cast<const std::string &>(options[OptFilter]); > - > - argv[argc] = const_cast<char *>(filterParam.c_str()); > - argc++; > + filterParam = "--gtest_filter=" + options[OptFilter].toString(); > + argv.push_back(const_cast<char *>(filterParam.c_str())); > } > > - argv[argc] = nullptr; > - > - ::testing::InitGoogleTest(&argc, argv); > + argv.push_back(nullptr); > > - delete[] argv; > + int argc = argv.size(); > + ::testing::InitGoogleTest(&argc, argv.data()); > > return 0; > }
Hi 2025. január 10., péntek 1:21 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > Hi Barnabás, > > Thank you for the patch. > > On Fri, Dec 20, 2024 at 03:08:29PM +0000, Barnabás Pőcze wrote: > > Just use an `std::vector` to store the arguments passed to > > `InitGoogleTest()`. This removes the need for the map and > > the separate `argc` variable used for size-keeping. > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > src/apps/lc-compliance/main.cpp | 36 +++++++++------------------------ > > 1 file changed, 9 insertions(+), 27 deletions(-) > > > > diff --git a/src/apps/lc-compliance/main.cpp b/src/apps/lc-compliance/main.cpp > > index cdd0bd515..3d9c51fc3 100644 > > --- a/src/apps/lc-compliance/main.cpp > > +++ b/src/apps/lc-compliance/main.cpp > > @@ -80,45 +80,27 @@ static int initCamera(CameraManager *cm, OptionsParser::Options options) > > > > static int initGtestParameters(char *arg0, OptionsParser::Options options) > > { > > - const std::map<std::string, std::string> gtestFlags = { { "list", "--gtest_list_tests" }, > > - { "filter", "--gtest_filter" } }; > > - > > - int argc = 0; > > + std::vector<char *> argv; > > std::string filterParam; > > > > - /* > > - * +2 to have space for both the 0th argument that is needed but not > > - * used and the null at the end. > > - */ > > - char **argv = new char *[(gtestFlags.size() + 2)]; > > - if (!argv) > > - return -ENOMEM; > > - > > - argv[0] = arg0; > > - argc++; > > + argv.push_back(arg0); > > > > - if (options.isSet(OptList)) { > > - argv[argc] = const_cast<char *>(gtestFlags.at("list").c_str()); > > - argc++; > > - } > > + if (options.isSet(OptList)) > > + argv.push_back(const_cast<char *>("--gtest_list_tests")); > > Functions that take a char ** when they never modify the contents of the > strings are annoying :-/ > > Would it be better to drop the const_char here and below, and add a > single one when calling ::testing::InitGoogleTest() with > > ::testing::InitGoogleTest(&argc, const_cast<char **>(argv.data())); > > ? Yes, that works, so I'll move the `const_cast` into the `InitGoogleTest()` invocation. Regards, Barnabás Pőcze > > Either way, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > if (options.isSet(OptFilter)) { > > /* > > * The filter flag needs to be passed as a single parameter, in > > * the format --gtest_filter=filterStr > > */ > > - filterParam = gtestFlags.at("filter") + "=" + > > - static_cast<const std::string &>(options[OptFilter]); > > - > > - argv[argc] = const_cast<char *>(filterParam.c_str()); > > - argc++; > > + filterParam = "--gtest_filter=" + options[OptFilter].toString(); > > + argv.push_back(const_cast<char *>(filterParam.c_str())); > > } > > > > - argv[argc] = nullptr; > > - > > - ::testing::InitGoogleTest(&argc, argv); > > + argv.push_back(nullptr); > > > > - delete[] argv; > > + int argc = argv.size(); > > + ::testing::InitGoogleTest(&argc, argv.data()); > > > > return 0; > > } > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/apps/lc-compliance/main.cpp b/src/apps/lc-compliance/main.cpp index cdd0bd515..3d9c51fc3 100644 --- a/src/apps/lc-compliance/main.cpp +++ b/src/apps/lc-compliance/main.cpp @@ -80,45 +80,27 @@ static int initCamera(CameraManager *cm, OptionsParser::Options options) static int initGtestParameters(char *arg0, OptionsParser::Options options) { - const std::map<std::string, std::string> gtestFlags = { { "list", "--gtest_list_tests" }, - { "filter", "--gtest_filter" } }; - - int argc = 0; + std::vector<char *> argv; std::string filterParam; - /* - * +2 to have space for both the 0th argument that is needed but not - * used and the null at the end. - */ - char **argv = new char *[(gtestFlags.size() + 2)]; - if (!argv) - return -ENOMEM; - - argv[0] = arg0; - argc++; + argv.push_back(arg0); - if (options.isSet(OptList)) { - argv[argc] = const_cast<char *>(gtestFlags.at("list").c_str()); - argc++; - } + if (options.isSet(OptList)) + argv.push_back(const_cast<char *>("--gtest_list_tests")); if (options.isSet(OptFilter)) { /* * The filter flag needs to be passed as a single parameter, in * the format --gtest_filter=filterStr */ - filterParam = gtestFlags.at("filter") + "=" + - static_cast<const std::string &>(options[OptFilter]); - - argv[argc] = const_cast<char *>(filterParam.c_str()); - argc++; + filterParam = "--gtest_filter=" + options[OptFilter].toString(); + argv.push_back(const_cast<char *>(filterParam.c_str())); } - argv[argc] = nullptr; - - ::testing::InitGoogleTest(&argc, argv); + argv.push_back(nullptr); - delete[] argv; + int argc = argv.size(); + ::testing::InitGoogleTest(&argc, argv.data()); return 0; }
Just use an `std::vector` to store the arguments passed to `InitGoogleTest()`. This removes the need for the map and the separate `argc` variable used for size-keeping. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/apps/lc-compliance/main.cpp | 36 +++++++++------------------------ 1 file changed, 9 insertions(+), 27 deletions(-)