From d284fc655cf5fb9af3e306ac1fcbcb13c01430a5 Mon Sep 17 00:00:00 2001 From: Marcel Schneider Date: Thu, 21 Dec 2017 12:56:53 +0100 Subject: [PATCH 1/5] calling exit(int) does not play well with RAII valgrind shows memory as still reachable --- cmdparser.hpp | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/cmdparser.hpp b/cmdparser.hpp index 65f1427..3a8f1e7 100644 --- a/cmdparser.hpp +++ b/cmdparser.hpp @@ -12,6 +12,9 @@ #include namespace cli { + class NormalExitFromCallback : std::exception{ + + }; /// Class used to wrap integer types to specify desired numerical base for specific argument parsing template class NumericalBase { public: @@ -106,7 +109,11 @@ namespace cli { CallbackArgs args { arguments, output, error }; value = callback(args); return true; - } catch (...) { + } + catch(NormalExitFromCallback const&){ + return true; + } + catch (...) { return false; } } @@ -130,7 +137,11 @@ namespace cli { try { value = Parser::parse(arguments, value); return true; - } catch (...) { + } + catch(NormalExitFromCallback const&){ + return true; + } + catch (...) { return false; } } @@ -296,7 +307,7 @@ namespace cli { void enable_help() { set_callback("h", "help", std::function([this](CallbackArgs& args){ args.output << this->usage(); - exit(0); + throw NormalExitFromCallback(); return false; }), "", true); } @@ -338,7 +349,7 @@ namespace cli { inline void run_and_exit_if_error() { if (run() == false) { - exit(1); + throw std::runtime_error(""); } } From 33fffb61c947a03c0cf9e1cd3b82bbcd65648464 Mon Sep 17 00:00:00 2001 From: Marcel Schneider Date: Thu, 21 Dec 2017 13:47:09 +0100 Subject: [PATCH 2/5] fixed TEST_CASE( "Parse help", "[help]" ) in this test value is expected to be false, but should this return true? --- cmdparser.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmdparser.hpp b/cmdparser.hpp index 3a8f1e7..c313ed4 100644 --- a/cmdparser.hpp +++ b/cmdparser.hpp @@ -111,7 +111,7 @@ namespace cli { return true; } catch(NormalExitFromCallback const&){ - return true; + return false;// should this return "true"? } catch (...) { return false; From 42320bf6587f8ba88e384ef0991358728238d506 Mon Sep 17 00:00:00 2001 From: Marcel Schneider Date: Thu, 21 Dec 2017 13:34:42 +0100 Subject: [PATCH 3/5] delete resource before erasing --- cmdparser.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/cmdparser.hpp b/cmdparser.hpp index c313ed4..a2065f2 100644 --- a/cmdparser.hpp +++ b/cmdparser.hpp @@ -315,6 +315,7 @@ namespace cli { void disable_help() { for (auto command = _commands.begin(); command != _commands.end(); ++command) { if ((*command)->name == "h" && (*command)->alternative == "--help") { + delete(*command); _commands.erase(command); break; } From a0a3a99c66204cecebbc9890c80a950a662b15b5 Mon Sep 17 00:00:00 2001 From: Marcel Schneider Date: Thu, 21 Dec 2017 14:30:24 +0100 Subject: [PATCH 4/5] Replace raw pointer with std::unique_ptr now manual resource management anymore --- cmdparser.hpp | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/cmdparser.hpp b/cmdparser.hpp index a2065f2..1326201 100644 --- a/cmdparser.hpp +++ b/cmdparser.hpp @@ -9,6 +9,7 @@ #include #include #include +#include #include namespace cli { @@ -288,14 +289,8 @@ namespace cli { enable_help(); } - ~Parser() { - for (int i = 0, n = _commands.size(); i < n; ++i) { - delete _commands[i]; - } - } - bool has_help() const { - for (const auto command : _commands) { + for (const auto &command : _commands) { if (command->name == "h" && command->alternative == "--help") { return true; } @@ -315,7 +310,6 @@ namespace cli { void disable_help() { for (auto command = _commands.begin(); command != _commands.end(); ++command) { if ((*command)->name == "h" && (*command)->alternative == "--help") { - delete(*command); _commands.erase(command); break; } @@ -330,22 +324,21 @@ namespace cli { template void set_required(const std::string& name, const std::string& alternative, const std::string& description = "", bool dominant = false) { - auto command = new CmdArgument { name, alternative, description, true, dominant }; - _commands.push_back(command); + _commands.emplace_back(new CmdArgument { name, alternative, description, true, dominant }); } template void set_optional(const std::string& name, const std::string& alternative, T defaultValue, const std::string& description = "", bool dominant = false) { auto command = new CmdArgument { name, alternative, description, false, dominant }; command->value = defaultValue; - _commands.push_back(command); + _commands.emplace_back(command); } template void set_callback(const std::string& name, const std::string& alternative, std::function callback, const std::string& description = "", bool dominant = false) { auto command = new CmdFunction { name, alternative, description, false, dominant }; command->callback = callback; - _commands.push_back(command); + _commands.emplace_back(command); } inline void run_and_exit_if_error() { @@ -392,25 +385,25 @@ namespace cli { // First, parse dominant arguments since they succeed even if required // arguments are missing. - for (auto command : _commands) { + for (auto &command : _commands) { if (command->handled && command->dominant && !command->parse(output, error)) { - error << howto_use(command); + error << howto_use(command.get()); return false; } } // Next, check for any missing arguments. - for (auto command : _commands) { + for (auto &command : _commands) { if (command->required && !command->handled) { - error << howto_required(command); + error << howto_required(command.get()); return false; } } // Finally, parse all remaining arguments. - for (auto command : _commands) { + for (auto &command : _commands) { if (command->handled && !command->dominant && !command->parse(output, error)) { - error << howto_use(command); + error << howto_use(command.get()); return false; } } @@ -422,7 +415,7 @@ namespace cli { T get(const std::string& name) const { for (const auto& command : _commands) { if (command->name == name) { - auto cmd = dynamic_cast*>(command); + auto cmd = dynamic_cast*>(command.get()); if (cmd == nullptr) { throw std::runtime_error("Invalid usage of the parameter " + name + " detected."); @@ -463,9 +456,9 @@ namespace cli { protected: CmdBase* find(const std::string& name) { - for (auto command : _commands) { + for (auto &command : _commands) { if (command->is(name)) { - return command; + return command.get(); } } @@ -473,9 +466,9 @@ namespace cli { } CmdBase* find_default() { - for (auto command : _commands) { + for (auto &command : _commands) { if (command->name == "") { - return command; + return command.get(); } } @@ -538,6 +531,6 @@ namespace cli { private: const std::string _appname; std::vector _arguments; - std::vector _commands; + std::vector> _commands; }; } From 09ea54b036354b15dc041d73210bce6aa04108d1 Mon Sep 17 00:00:00 2001 From: Marcel Schneider Date: Thu, 21 Dec 2017 19:22:11 +0100 Subject: [PATCH 5/5] added target sample for make this helped me figuring out that Catch itself includes and the tests compiled without including memory in CmdParser.hpp to compile this, type make sample now cmdparser.hpp is the first include in tests --- cmdparser.Test/CMakeLists.txt | 1 + cmdparser.Test/sample.cpp | 15 +++++++++++++++ cmdparser.Test/tests.cpp | 2 +- 3 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 cmdparser.Test/sample.cpp diff --git a/cmdparser.Test/CMakeLists.txt b/cmdparser.Test/CMakeLists.txt index 9dee61c..a7a815a 100644 --- a/cmdparser.Test/CMakeLists.txt +++ b/cmdparser.Test/CMakeLists.txt @@ -1,5 +1,6 @@ set(SOURCE_FILES TestMain.cpp catch.hpp tests.cpp) add_executable(cmdparserTest ${SOURCE_FILES}) +add_executable(sample sample.cpp) IF(APPLE) TARGET_COMPILE_OPTIONS(cmdparserTest PUBLIC INTERFACE "-stdlib=libc++") ENDIF(APPLE) \ No newline at end of file diff --git a/cmdparser.Test/sample.cpp b/cmdparser.Test/sample.cpp new file mode 100644 index 0000000..00eebb8 --- /dev/null +++ b/cmdparser.Test/sample.cpp @@ -0,0 +1,15 @@ +// +// Created by marcel on 12/21/17. +// + +#include +#include "../cmdparser.hpp" + +int main(int argc, char**argv) +{ + cli::Parser parser(argc, argv); + parser.disable_help(); + const auto value = parser.run(std::cout, std::cerr); + + return 0; +} diff --git a/cmdparser.Test/tests.cpp b/cmdparser.Test/tests.cpp index 5a44fb7..f1cbd09 100644 --- a/cmdparser.Test/tests.cpp +++ b/cmdparser.Test/tests.cpp @@ -3,9 +3,9 @@ Copyright (c) 2015 - 2016 Florian Rappl */ -#include "catch.hpp" #include #include "../cmdparser.hpp" +#include "catch.hpp" using namespace cli;