add additional tests for coverage, (#928)

Add some additional tests to try to get test coverage back to 100%

 refactor the positional parsing to  reduce code duplication

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

cmake/CodeCoverage.cmake

@@ -88,7 +88,7 @@ elseif(NOT CMAKE_COMPILER_IS_GNUCXX)
 endif()
 
 set(COVERAGE_COMPILER_FLAGS
-    "-g -O0 --coverage -fprofile-arcs -ftest-coverage -fno-inline -fno-inline-small-functions -fno-default-inline"
+    "-g -O0 --coverage -fprofile-arcs -ftest-coverage -fno-inline -fno-inline-small-functions -fno-default-inline -fno-elide-constructors"
     CACHE INTERNAL "")
 
 set(CMAKE_CXX_FLAGS_COVERAGE

include/CLI/impl/App_inl.hpp

@@ -82,7 +82,7 @@ CLI11_NODISCARD CLI11_INLINE char **App::ensure_utf8(char **argv) {
 CLI11_INLINE App *App::name(std::string app_name) {
 
     if(parent_ != nullptr) {
-        auto oname = name_;
+        std::string oname = name_;
         name_ = app_name;
         const auto &res = _compare_subcommand_names(*this, *_get_fallthrough_parent());
         if(!res.empty()) {
@@ -832,7 +832,7 @@ CLI11_NODISCARD CLI11_INLINE bool App::check_name(std::string name_to_check) con
     if(local_name == name_to_check) {
         return true;
     }
-    for(auto les : aliases_) {  // NOLINT(performance-for-range-copy)
+    for(std::string les : aliases_) {  // NOLINT(performance-for-range-copy)
         if(ignore_underscore_) {
             les = detail::remove_underscore(les);
         }
@@ -1556,6 +1556,7 @@ CLI11_NODISCARD CLI11_INLINE bool App::_has_remaining_positionals() const {
 CLI11_INLINE bool App::_parse_positional(std::vector<std::string> &args, bool haltOnSubcommand) {
 
     const std::string &positional = args.back();
+    Option *posOpt{nullptr};
 
     if(positionals_at_end_) {
         // deal with the case of required arguments at the end which should take precedence over other arguments
@@ -1572,57 +1573,48 @@ CLI11_INLINE bool App::_parse_positional(std::vector<std::string> &args, bool ha
                                 continue;
                             }
                         }
-
+                        posOpt = opt.get();
-                        parse_order_.push_back(opt.get());
+                        break;
-                        /// if we require a separator add it here
-                        if(opt->get_inject_separator()) {
-                            if(!opt->results().empty() && !opt->results().back().empty()) {
-                                opt->add_result(std::string{});
-                            }
-                        }
-                        if(opt->get_trigger_on_parse() &&
-                           opt->current_option_state_ == Option::option_state::callback_run) {
-                            opt->clear();
-                        }
-                        opt->add_result(positional);
-                        if(opt->get_trigger_on_parse()) {
-                            opt->run_callback();
-                        }
-                        args.pop_back();
-                        return true;
                     }
                 }
             }
         }
     }
-    for(const Option_p &opt : options_) {
+    if(posOpt == nullptr) {
-        // Eat options, one by one, until done
+        for(const Option_p &opt : options_) {
-        if(opt->get_positional() &&
+            // Eat options, one by one, until done
-           (static_cast<int>(opt->count()) < opt->get_items_expected_min() || opt->get_allow_extra_args())) {
+            if(opt->get_positional() &&
-            if(validate_positionals_) {
+               (static_cast<int>(opt->count()) < opt->get_items_expected_min() || opt->get_allow_extra_args())) {
-                std::string pos = positional;
+                if(validate_positionals_) {
-                pos = opt->_validate(pos, 0);
+                    std::string pos = positional;
-                if(!pos.empty()) {
+                    pos = opt->_validate(pos, 0);
-                    continue;
+                    if(!pos.empty()) {
+                        continue;
+                    }
                 }
+                posOpt = opt.get();
+                break;
             }
-            if(opt->get_inject_separator()) {
-                if(!opt->results().empty() && !opt->results().back().empty()) {
-                    opt->add_result(std::string{});
-                }
-            }
-            if(opt->get_trigger_on_parse() && opt->current_option_state_ == Option::option_state::callback_run) {
-                opt->clear();
-            }
-            opt->add_result(positional);
-            if(opt->get_trigger_on_parse()) {
-                opt->run_callback();
-            }
-            parse_order_.push_back(opt.get());
-            args.pop_back();
-            return true;
         }
     }
+    if(posOpt != nullptr) {
+        parse_order_.push_back(posOpt);
+        if(posOpt->get_inject_separator()) {
+            if(!posOpt->results().empty() && !posOpt->results().back().empty()) {
+                posOpt->add_result(std::string{});
+            }
+        }
+        if(posOpt->get_trigger_on_parse() && posOpt->current_option_state_ == Option::option_state::callback_run) {
+            posOpt->clear();
+        }
+        posOpt->add_result(positional);
+        if(posOpt->get_trigger_on_parse()) {
+            posOpt->run_callback();
+        }
+
+        args.pop_back();
+        return true;
+    }
 
     for(auto &subc : subcommands_) {
         if((subc->name_.empty()) && (!subc->disabled_)) {
@@ -1964,7 +1956,7 @@ CLI11_INLINE void App::_trigger_pre_parse(std::size_t remaining_args) {
     } else if(immediate_callback_) {
         if(!name_.empty()) {
             auto pcnt = parsed_;
-            auto extras = std::move(missing_);
+            missing_t extras = std::move(missing_);
             clear();
             parsed_ = pcnt;
             pre_parse_called_ = true;
@@ -2150,12 +2142,12 @@ CLI11_INLINE void retire_option(App *app, Option *opt) {
                             ->allow_extra_args(opt->get_allow_extra_args());
 
     app->remove_option(opt);
-    auto *opt2 = app->add_option(option_copy->get_name(false, true), "option has been retired and has no effect")
+    auto *opt2 = app->add_option(option_copy->get_name(false, true), "option has been retired and has no effect");
-                     ->type_name("RETIRED")
+    opt2->type_name("RETIRED")
-                     ->default_str("RETIRED")
+        ->default_str("RETIRED")
-                     ->type_size(option_copy->get_type_size_min(), option_copy->get_type_size_max())
+        ->type_size(option_copy->get_type_size_min(), option_copy->get_type_size_max())
-                     ->expected(option_copy->get_expected_min(), option_copy->get_expected_max())
+        ->expected(option_copy->get_expected_min(), option_copy->get_expected_max())
-                     ->allow_extra_args(option_copy->get_allow_extra_args());
+        ->allow_extra_args(option_copy->get_allow_extra_args());
 
     Validator retired_warning{[opt2](std::string &) {
                                   std::cout << "WARNING " << opt2->get_name() << " is retired and has no effect\n";

include/CLI/impl/Config_inl.hpp

@@ -221,15 +221,15 @@ inline std::vector<ConfigItem> ConfigBase::from_config(std::istream &input) cons
         }
 
         // Find = in string, split and recombine
-        auto pos = line.find(valueDelimiter);
+        auto delimiter_pos = line.find_first_of(valueDelimiter);
-        if(pos != std::string::npos) {
+        auto comment_pos = line.find_first_of(commentChar);
-            name = detail::trim_copy(line.substr(0, pos));
+        if(comment_pos < delimiter_pos) {
-            std::string item = detail::trim_copy(line.substr(pos + 1));
+            delimiter_pos = std::string::npos;
-            auto cloc = item.find(commentChar);
+        }
-            if(cloc != std::string::npos) {
+        if(delimiter_pos != std::string::npos) {
-                item.erase(cloc, std::string::npos);  // NOLINT(readability-suspicious-call-argument)
+            name = detail::trim_copy(line.substr(0, delimiter_pos));
-                detail::trim(item);
+            std::string item = detail::trim_copy(line.substr(delimiter_pos + 1, comment_pos - delimiter_pos - 1));
-            }
+
             if(item.size() > 1 && item.front() == aStart) {
                 for(std::string multiline; item.back() != aEnd && std::getline(input, multiline);) {
                     detail::trim(multiline);
@@ -244,13 +244,7 @@ inline std::vector<ConfigItem> ConfigBase::from_config(std::istream &input) cons
                 items_buffer = {item};
             }
         } else {
-            name = detail::trim_copy(line);
+            name = detail::trim_copy(line.substr(0, comment_pos));
-            auto cloc = name.find(commentChar);
-            if(cloc != std::string::npos) {
-                name.erase(cloc, std::string::npos);  // NOLINT(readability-suspicious-call-argument)
-                detail::trim(name);
-            }
-
             items_buffer = {"true"};
         }
         if(name.find(parentSeparatorChar) == std::string::npos) {

tests/AppTest.cpp

@@ -27,7 +27,7 @@ TEST_CASE_METHOD(TApp, "OneFlagShortValues", "[app]") {
     run();
     CHECK(app.count("-c") == 1u);
     CHECK(app.count("--count") == 1u);
-    auto v = app["-c"]->results();
+    const auto &v = app["-c"]->results();
     CHECK("v1" == v[0]);
 
     CHECK_THROWS_AS(app["--invalid"], CLI::OptionNotFound);
@@ -1175,6 +1175,21 @@ TEST_CASE_METHOD(TApp, "PositionalAtEnd", "[app]") {
     CHECK_THROWS_AS(run(), CLI::ExtrasError);
 }
 
+// Tests positionals at end
+TEST_CASE_METHOD(TApp, "PositionalInjectSeparator", "[app]") {
+    std::string options;
+    std::vector<std::vector<std::string>> foo;
+
+    app.add_option("-O", options);
+    auto *fooopt = app.add_option("foo", foo);
+    fooopt->inject_separator();
+    args = {"test1", "-O", "Test", "test2"};
+    run();
+
+    CHECK("Test" == options);
+    CHECK(foo.size() == 2U);
+}
+
 // Tests positionals at end
 TEST_CASE_METHOD(TApp, "RequiredPositionals", "[app]") {
     std::vector<std::string> sources;

tests/ConfigFileTest.cpp

@@ -84,6 +84,7 @@ TEST_CASE("StringBased: FirstWithComments", "[config]") {
     ofile << "one=three\n";
     ofile << "two=four\n";
     ofile << "; and another one\n";
+    ofile << "   ; and yet another one\n";
 
     ofile.seekg(0, std::ios::beg);
 
@@ -1804,6 +1805,8 @@ TEST_CASE_METHOD(TApp, "IniNotConfigurable", "[config]") {
     }
 
     CHECK_THROWS_AS(run(), CLI::ConfigError);
+    app.allow_config_extras(CLI::config_extras_mode::ignore_all);
+    CHECK_NOTHROW(run());
 }
 
 TEST_CASE_METHOD(TApp, "IniSubFailure", "[config]") {
@@ -2072,6 +2075,66 @@ TEST_CASE_METHOD(TApp, "IniFlags", "[config]") {
     CHECK(five);
 }
 
+TEST_CASE_METHOD(TApp, "IniFlagsComment", "[config]") {
+    TempFile tmpini{"TestIniTmp.ini"};
+    app.set_config("--config", tmpini);
+
+    {
+        std::ofstream out{tmpini};
+        out << "[default]" << std::endl;
+        out << "two=2 # comment 1" << std::endl;
+        out << "three=true" << std::endl;
+        out << "four=on #comment 2" << std::endl;
+        out << "five #comment 3" << std::endl;
+        out << std::endl;
+    }
+
+    int two{0};
+    bool three{false}, four{false}, five{false};
+    app.add_flag("--two", two);
+    app.add_flag("--three", three);
+    app.add_flag("--four", four);
+    app.add_flag("--five", five);
+
+    run();
+
+    CHECK(two == 2);
+    CHECK(three);
+    CHECK(four);
+    CHECK(five);
+}
+
+TEST_CASE_METHOD(TApp, "IniFlagsAltComment", "[config]") {
+    TempFile tmpini{"TestIniTmp.ini"};
+    app.set_config("--config", tmpini);
+
+    {
+        std::ofstream out{tmpini};
+        out << "[default]" << std::endl;
+        out << "two=2 % comment 1" << std::endl;
+        out << "three=true" << std::endl;
+        out << "four=on %% comment 2" << std::endl;
+        out << "five %= 3" << std::endl;
+        out << std::endl;
+    }
+
+    auto config = app.get_config_formatter_base();
+    config->comment('%');
+    int two{0};
+    bool three{false}, four{false}, five{false};
+    app.add_flag("--two", two);
+    app.add_flag("--three", three);
+    app.add_flag("--four", four);
+    app.add_flag("--five", five);
+
+    run();
+
+    CHECK(two == 2);
+    CHECK(three);
+    CHECK(four);
+    CHECK(five);
+}
+
 TEST_CASE_METHOD(TApp, "IniFalseFlags", "[config]") {
     TempFile tmpini{"TestIniTmp.ini"};
     app.set_config("--config", tmpini);
@@ -2299,6 +2362,17 @@ TEST_CASE_METHOD(TApp, "TomlOutputShortSingleDescription", "[config]") {
     CHECK_THAT(str, Contains("# " + description + "\n" + flag + "=false\n"));
 }
 
+TEST_CASE_METHOD(TApp, "TomlOutputdefaultOptionString", "[config]") {
+    std::string option = "some_option";
+    const std::string description = "Some short description.";
+    app.add_option("--" + option, description)->run_callback_for_default();
+
+    run();
+
+    std::string str = app.config_to_str(true, true);
+    CHECK_THAT(str, Contains("# " + description + "\n" + option + "=\"\"\n"));
+}
+
 TEST_CASE_METHOD(TApp, "TomlOutputShortDoubleDescription", "[config]") {
     std::string flag1 = "flagnr1";
     std::string flag2 = "flagnr2";

tests/HelpTest.cpp

@@ -1336,3 +1336,19 @@ TEST_CASE("TVersion: parse_throw", "[help]") {
         CHECK(1U == cptr->count());
     }
 }
+
+TEST_CASE("TVersion: exit", "[help]") {
+
+    CLI::App app;
+
+    app.set_version_flag("--version", CLI11_VERSION);
+
+    try {
+        app.parse("--version");
+    } catch(const CLI::CallForVersion &v) {
+        std::ostringstream out;
+        auto ret = app.exit(v, out);
+        CHECK_THAT(out.str(), Contains(CLI11_VERSION));
+        CHECK(0 == ret);
+    }
+}