From f84fa8484a3c69705738c4e3cd0c2ae1f844dfd2 Mon Sep 17 00:00:00 2001 From: Pranav Srinivas Kumar Date: Sun, 5 Nov 2023 18:13:17 -0600 Subject: [PATCH] Marked copy and move constructors as deleted --- README.md | 10 ++- include/argparse/argparse.hpp | 65 ++++------------ test/CMakeLists.txt | 2 - test/test_const_correct.cpp | 31 -------- test/test_mutually_exclusive_group.cpp | 17 ----- test/test_value_semantics.cpp | 100 ------------------------- 6 files changed, 22 insertions(+), 203 deletions(-) delete mode 100644 test/test_const_correct.cpp delete mode 100644 test/test_value_semantics.cpp diff --git a/README.md b/README.md index 202ff119..403995b3 100644 --- a/README.md +++ b/README.md @@ -46,6 +46,8 @@ * [Positional Arguments with Compound Toggle Arguments](#positional-arguments-with-compound-toggle-arguments) * [Restricting the set of values for an argument](#restricting-the-set-of-values-for-an-argument) * [Using `option=value` syntax](#using-optionvalue-syntax) +* [Developer Notes](#developer-notes) + * [Copying and Moving](#copying-and-moving) * [CMake Integration](#cmake-integration) * [Building, Installing, and Testing](#building-installing-and-testing) * [Supported Toolchains](#supported-toolchains) @@ -1198,7 +1200,7 @@ foo@bar:/home/dev/$ ./main 6 Invalid argument "6" - allowed options: {0, 1, 2, 3, 4, 5} ``` -## Using `option=value` syntax +### Using `option=value` syntax ```cpp #include "argparse.hpp" @@ -1234,6 +1236,12 @@ foo@bar:/home/dev/$ ./test --bar=BAR --foo --bar: BAR ``` +## Developer Notes + +### Copying and Moving + +`argparse::ArgumentParser` is intended to be used in a single function - setup everything and parse arguments in one place. Attempting to move or copy invalidates internal references (issue #260). Thus, starting with v3.0, `argparse::ArgumentParser` copy and move constructors are marked as `delete`. + ## CMake Integration Use the latest argparse in your CMake project without copying any content. diff --git a/include/argparse/argparse.hpp b/include/argparse/argparse.hpp index 22fffc07..27ab3e42 100644 --- a/include/argparse/argparse.hpp +++ b/include/argparse/argparse.hpp @@ -1395,53 +1395,18 @@ class ArgumentParser { } } - ArgumentParser(ArgumentParser &&) noexcept = default; - ArgumentParser &operator=(ArgumentParser &&) = default; - - ArgumentParser(const ArgumentParser &other) - : m_program_name(other.m_program_name), m_version(other.m_version), - m_description(other.m_description), m_epilog(other.m_epilog), - m_prefix_chars(other.m_prefix_chars), - m_assign_chars(other.m_assign_chars), m_is_parsed(other.m_is_parsed), - m_positional_arguments(other.m_positional_arguments), - m_optional_arguments(other.m_optional_arguments), - m_parser_path(other.m_parser_path), m_subparsers(other.m_subparsers), - m_suppress(other.m_suppress) { - for (auto it = std::begin(m_positional_arguments); - it != std::end(m_positional_arguments); ++it) { - index_argument(it); - } - for (auto it = std::begin(m_optional_arguments); - it != std::end(m_optional_arguments); ++it) { - index_argument(it); - } - for (auto it = std::begin(m_subparsers); it != std::end(m_subparsers); - ++it) { - m_subparser_map.insert_or_assign(it->get().m_program_name, it); - m_subparser_used.insert_or_assign(it->get().m_program_name, false); - } - - for (const auto &g : other.m_mutually_exclusive_groups) { - MutuallyExclusiveGroup group(*this, g.m_required); - for (const auto &arg : g.m_elements) { - // Find argument in argument map and add reference to it - // in new group - // argument_it = other.m_argument_map.find("name") - auto first_name = arg->m_names[0]; - auto it = m_argument_map.find(first_name); - group.m_elements.push_back(&(*it->second)); - } - m_mutually_exclusive_groups.push_back(std::move(group)); - } - } - ~ArgumentParser() = default; - ArgumentParser &operator=(const ArgumentParser &other) { - auto tmp = other; - std::swap(*this, tmp); - return *this; - } + // ArgumentParser is meant to be used in a single function. + // Setup everything and parse arguments in one place. + // + // ArgumentParser internally uses std::string_views, + // references, iterators, etc. + // Many of these elements become invalidated after a copy or move. + ArgumentParser(const ArgumentParser &other) = delete; + ArgumentParser &operator=(const ArgumentParser &other) = delete; + ArgumentParser(ArgumentParser &&) noexcept = delete; + ArgumentParser &operator=(ArgumentParser &&) = delete; explicit operator bool() const { auto arg_used = std::any_of(m_argument_map.cbegin(), m_argument_map.cend(), @@ -1749,10 +1714,8 @@ class ArgumentParser { } bool has_visible_subcommands = std::any_of( - parser.m_subparser_map.begin(), - parser.m_subparser_map.end(), - [] (auto &p) { return !p.second->get().m_suppress; } - ); + parser.m_subparser_map.begin(), parser.m_subparser_map.end(), + [](auto &p) { return !p.second->get().m_suppress; }); if (has_visible_subcommands) { stream << (parser.m_positional_arguments.empty() @@ -1842,9 +1805,7 @@ class ArgumentParser { m_subparser_used.insert_or_assign(parser.m_program_name, false); } - void set_suppress(bool suppress) { - m_suppress = suppress; - } + void set_suppress(bool suppress) { m_suppress = suppress; } private: bool is_valid_prefix_char(char c) const { diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 8e942be2..587eb824 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -32,7 +32,6 @@ file(GLOB ARGPARSE_TEST_SOURCES test_choices.cpp test_compound_arguments.cpp test_container_arguments.cpp - test_const_correct.cpp test_default_args.cpp test_default_value.cpp test_error_reporting.cpp @@ -51,7 +50,6 @@ file(GLOB ARGPARSE_TEST_SOURCES test_required_arguments.cpp test_scan.cpp test_stringstream.cpp - test_value_semantics.cpp test_version.cpp test_subparsers.cpp test_parse_known_args.cpp diff --git a/test/test_const_correct.cpp b/test/test_const_correct.cpp deleted file mode 100644 index a139679b..00000000 --- a/test/test_const_correct.cpp +++ /dev/null @@ -1,31 +0,0 @@ -#ifdef WITH_MODULE -import argparse; -#else -#include -#endif -#include - -using doctest::test_suite; - -TEST_CASE("ArgumentParser is const-correct after construction and parsing" * - test_suite("value_semantics")) { - GIVEN("a parser") { - argparse::ArgumentParser parser("test"); - parser.add_argument("--foo", "-f").help("I am foo"); - parser.add_description("A description"); - parser.add_epilog("An epilog"); - - WHEN("becomes const-qualified") { - parser.parse_args({"./main", "--foo", "baz"}); - const auto const_parser = std::move(parser); - - THEN("only const methods are accessible") { - REQUIRE(const_parser.help().str().size() > 0); - REQUIRE(const_parser.present("--foo")); - REQUIRE(const_parser.is_used("-f")); - REQUIRE(const_parser.get("-f") == "baz"); - REQUIRE(const_parser["-f"] == std::string("baz")); - } - } - } -} diff --git a/test/test_mutually_exclusive_group.cpp b/test/test_mutually_exclusive_group.cpp index 93d62157..f2433647 100644 --- a/test/test_mutually_exclusive_group.cpp +++ b/test/test_mutually_exclusive_group.cpp @@ -52,23 +52,6 @@ TEST_CASE( std::runtime_error); } -TEST_CASE( - "Create mutually exclusive group with 2 arguments, then copy the parser" * - test_suite("mutex_args")) { - argparse::ArgumentParser program("test"); - - auto &group = program.add_mutually_exclusive_group(); - group.add_argument("--first"); - group.add_argument("--second"); - - auto program_copy(program); - - REQUIRE_THROWS_WITH_AS( - program_copy.parse_args({"test", "--first", "1", "--second", "2"}), - "Argument '--second VAR' not allowed with '--first VAR'", - std::runtime_error); -} - TEST_CASE("Create mutually exclusive group with 3 arguments" * test_suite("mutex_args")) { argparse::ArgumentParser program("test"); diff --git a/test/test_value_semantics.cpp b/test/test_value_semantics.cpp deleted file mode 100644 index 03e5db5a..00000000 --- a/test/test_value_semantics.cpp +++ /dev/null @@ -1,100 +0,0 @@ -#ifdef WITH_MODULE -import argparse; -#else -#include -#endif -#include - -using doctest::test_suite; - -TEST_CASE("ArgumentParser is MoveConstructible and MoveAssignable" * - test_suite("value_semantics")) { - GIVEN("a parser that has two arguments") { - argparse::ArgumentParser parser("test"); - parser.add_argument("foo"); - parser.add_argument("-f"); - - WHEN("move construct a new parser from it") { - auto new_parser = std::move(parser); - - THEN("the old parser replaces the new parser") { - new_parser.parse_args({"test", "bar", "-f", "nul"}); - - REQUIRE(new_parser.get("foo") == "bar"); - REQUIRE(new_parser.get("-f") == "nul"); - } - } - - WHEN("move assign a parser prvalue to it") { - parser = argparse::ArgumentParser("test"); - - THEN("the old parser is replaced") { - REQUIRE_THROWS_AS(parser.parse_args({"test", "bar"}), - std::runtime_error); - REQUIRE_THROWS_AS(parser.parse_args({"test", "-f", "nul"}), - std::runtime_error); - REQUIRE_NOTHROW(parser.parse_args({"test"})); - } - } - } -} - -TEST_CASE("ArgumentParser is CopyConstructible and CopyAssignable" * - test_suite("value_semantics")) { - GIVEN("a parser that has two arguments") { - argparse::ArgumentParser parser("test"); - parser.add_argument("foo"); - parser.add_argument("-f"); - - WHEN("copy construct a new parser from it") { - auto new_parser = parser; - - THEN("the new parser has the old parser's capability") { - new_parser.parse_args({"test", "bar", "-f", "nul"}); - - REQUIRE(new_parser.get("foo") == "bar"); - REQUIRE(new_parser.get("-f") == "nul"); - - AND_THEN("but does not share states with the old parser") { - REQUIRE_THROWS_AS(parser.get("foo"), std::logic_error); - REQUIRE_THROWS_AS(parser.get("-f"), std::logic_error); - } - } - - AND_THEN("the old parser works as a distinct copy") { - new_parser.parse_args({"test", "toe", "-f", "/"}); - - REQUIRE(new_parser.get("foo") == "toe"); - REQUIRE(new_parser.get("-f") == "/"); - } - } - - WHEN("copy assign a parser lvalue to it") { - argparse::ArgumentParser optional_parser("test"); - optional_parser.add_argument("-g"); - parser = optional_parser; - - THEN("the old parser is replaced") { - REQUIRE_THROWS_AS(parser.parse_args({"test", "bar"}), - std::runtime_error); - REQUIRE_THROWS_AS(parser.parse_args({"test", "-f", "nul"}), - std::runtime_error); - REQUIRE_NOTHROW(parser.parse_args({"test"})); - REQUIRE_NOTHROW(parser.parse_args({"test", "-g", "nul"})); - - AND_THEN("but does not share states with the other parser") { - REQUIRE(parser.get("-g") == "nul"); - REQUIRE_THROWS_AS(optional_parser.get("-g"), std::logic_error); - } - } - - AND_THEN("the other parser works as a distinct copy") { - REQUIRE_NOTHROW(optional_parser.parse_args({"test"})); - REQUIRE_NOTHROW(optional_parser.parse_args({"test", "-g", "nul"})); - REQUIRE_THROWS_AS( - optional_parser.parse_args({"test", "bar", "-g", "nul"}), - std::runtime_error); - } - } - } -}