Merge pull request #304 from p-ranav/bugfix/260_segfault_copy

Marked copy and move constructors as deleted

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.  

include/argparse/argparse.hpp

@@ -1395,53 +1395,18 @@ public:
     }
   }
 
-  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 @@ public:
     }
 
     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 @@ public:
     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 {

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

test/test_const_correct.cpp

@@ -1,31 +0,0 @@
-#ifdef WITH_MODULE
-import argparse;
-#else
-#include <argparse/argparse.hpp>
-#endif
-#include <doctest.hpp>
-
-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<std::string>("--foo"));
-        REQUIRE(const_parser.is_used("-f"));
-        REQUIRE(const_parser.get("-f") == "baz");
-        REQUIRE(const_parser["-f"] == std::string("baz"));
-      }
-    }
-  }
-}

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");

test/test_value_semantics.cpp

@@ -1,100 +0,0 @@
-#ifdef WITH_MODULE
-import argparse;
-#else
-#include <argparse/argparse.hpp>
-#endif
-#include <doctest.hpp>
-
-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);
-      }
-    }
-  }
-}