From 8ced45959ffb744bb109a0fa329593b4c0fc5669 Mon Sep 17 00:00:00 2001 From: Markus Blatt Date: Tue, 14 Apr 2015 09:31:33 +0200 Subject: [PATCH 1/8] Correct documentation of why we use operator[] to initialize map. This commit updates the source code comment about using operator[] to initialize the unordered map. Thanks to Bard's persistence we found out that the cause is not the construction of the key value of type std::string from const char* but the mapped type being a (mutable) char* (due to C?). This completes the PR #784. --- opm/core/linalg/LinearSolverPetsc.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/opm/core/linalg/LinearSolverPetsc.cpp b/opm/core/linalg/LinearSolverPetsc.cpp index 3394ff70..044962d8 100644 --- a/opm/core/linalg/LinearSolverPetsc.cpp +++ b/opm/core/linalg/LinearSolverPetsc.cpp @@ -38,8 +38,10 @@ namespace{ : default_type_(default_type) { // g++-4.4 has problems converting const char* to char* - // which it thinks is needed for constructing std::string. - // Using operator[] circumvents this problem. + // The problem is caused by the mapped type being PCType + // which (at least in PETSc 3.2) is char* because of C + // (in the header there is "#define PCType character*(80)"). + // and the KSP... defines being const char* (because of C++). type_map_["richardson"] = KSPRICHARDSON; // Not available in PETSC 3.2 on Debian //type_map_["chebyshev"] = KSPCHEBYSHEV; From 1fb2bc651c910e241f1b36bd4cabc3f762b35e17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Atgeirr=20Fl=C3=B8=20Rasmussen?= Date: Tue, 14 Apr 2015 13:39:43 +0200 Subject: [PATCH 2/8] Store unhandled command-line arguments. A new method ParameterGroup::unhandledArguments() is available to access the list of unhandled arguments. Before, when such arguments were encountered they were ignored and a warning was printed to standard out. Apart from the lack of a (potentially misleading) warning, this should not change the behaviour of existing clients of the class. --- opm/core/utility/parameters/ParameterGroup.cpp | 5 +++++ opm/core/utility/parameters/ParameterGroup.hpp | 4 ++++ opm/core/utility/parameters/ParameterGroup_impl.hpp | 4 +--- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/opm/core/utility/parameters/ParameterGroup.cpp b/opm/core/utility/parameters/ParameterGroup.cpp index a6d7837d..21f642df 100644 --- a/opm/core/utility/parameters/ParameterGroup.cpp +++ b/opm/core/utility/parameters/ParameterGroup.cpp @@ -329,5 +329,10 @@ namespace Opm { } } + const std::vector ParameterGroup::unhandledArguments() const + { + return unhandled_arguments_; + } + } // namespace parameter } // namespace Opm diff --git a/opm/core/utility/parameters/ParameterGroup.hpp b/opm/core/utility/parameters/ParameterGroup.hpp index 466d242c..a4368805 100644 --- a/opm/core/utility/parameters/ParameterGroup.hpp +++ b/opm/core/utility/parameters/ParameterGroup.hpp @@ -267,6 +267,9 @@ namespace Opm { /// Insert a new parameter item into the group. void insertParameter(const std::string& name, const std::string& value); + /// Unhandled arguments from command line parsing. + const std::vector unhandledArguments() const; + private: typedef std::shared_ptr data_type; typedef std::pair pair_type; @@ -276,6 +279,7 @@ namespace Opm { map_type map_; const ParameterGroup* parent_; bool output_is_enabled_; + std::vector unhandled_arguments_; template T translate(const pair_type& data, const Requirement& chk) const; diff --git a/opm/core/utility/parameters/ParameterGroup_impl.hpp b/opm/core/utility/parameters/ParameterGroup_impl.hpp index eb8ebb1b..96deee4a 100644 --- a/opm/core/utility/parameters/ParameterGroup_impl.hpp +++ b/opm/core/utility/parameters/ParameterGroup_impl.hpp @@ -150,9 +150,7 @@ namespace Opm { } else if (file_type.second == "param") { this->readParam(files[i]); } else { - std::cout << "WARNING: Ignoring file '" - << files[i] << "' with unknown extension.\n" - << "Valid filename extensions are 'xml' and 'param'.\n"; + unhandled_arguments_.push_back(files[i]); } } for (int i = 0; i < int(assignments.size()); ++i) { From d4c0631c1d9ecdfd4ecd3fbff6528449ad195209 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Atgeirr=20Fl=C3=B8=20Rasmussen?= Date: Tue, 14 Apr 2015 13:46:30 +0200 Subject: [PATCH 3/8] Add test for ParameterGroup::unhandledArgument(). --- tests/test_param.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_param.cpp b/tests/test_param.cpp index afb0cc51..b9592dc7 100644 --- a/tests/test_param.cpp +++ b/tests/test_param.cpp @@ -85,7 +85,8 @@ BOOST_AUTO_TEST_CASE(xml_syntax_init) typedef const char* cp; cp argv[] = { "program_command", "testdata.xml", - "/group/item=overridingstring" }; + "/group/item=overridingstring", + "unhandledargument" }; const std::size_t argc = sizeof(argv)/sizeof(argv[0]); parameter::ParameterGroup p(argc, argv); BOOST_CHECK(p.get("topitem") == "somestring"); @@ -98,7 +99,8 @@ BOOST_AUTO_TEST_CASE(xml_syntax_init) "/slashtopitem=anotherstring\n" "/topitem=somestring\n"; BOOST_CHECK(os.str() == correct_answer); - + BOOST_REQUIRE(p.unhandledArguments().size() == 1); + BOOST_CHECK_EQUAL(p.unhandledArguments()[0], "unhandledargument"); // Tests that only run in debug mode. #ifndef NDEBUG #endif From dfe6e730ebe596a1e533bdaf181c406bdd6d7897 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Atgeirr=20Fl=C3=B8=20Rasmussen?= Date: Tue, 14 Apr 2015 13:55:29 +0200 Subject: [PATCH 4/8] Return const reference from unhandledArguments(). --- opm/core/utility/parameters/ParameterGroup.cpp | 2 +- opm/core/utility/parameters/ParameterGroup.hpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/opm/core/utility/parameters/ParameterGroup.cpp b/opm/core/utility/parameters/ParameterGroup.cpp index 21f642df..ab4d7ad5 100644 --- a/opm/core/utility/parameters/ParameterGroup.cpp +++ b/opm/core/utility/parameters/ParameterGroup.cpp @@ -329,7 +329,7 @@ namespace Opm { } } - const std::vector ParameterGroup::unhandledArguments() const + const std::vector& ParameterGroup::unhandledArguments() const { return unhandled_arguments_; } diff --git a/opm/core/utility/parameters/ParameterGroup.hpp b/opm/core/utility/parameters/ParameterGroup.hpp index a4368805..caecf4ad 100644 --- a/opm/core/utility/parameters/ParameterGroup.hpp +++ b/opm/core/utility/parameters/ParameterGroup.hpp @@ -268,7 +268,7 @@ namespace Opm { void insertParameter(const std::string& name, const std::string& value); /// Unhandled arguments from command line parsing. - const std::vector unhandledArguments() const; + const std::vector& unhandledArguments() const; private: typedef std::shared_ptr data_type; @@ -279,7 +279,7 @@ namespace Opm { map_type map_; const ParameterGroup* parent_; bool output_is_enabled_; - std::vector unhandled_arguments_; + std::vector unhandled_arguments_; template T translate(const pair_type& data, const Requirement& chk) const; From f902b7e2654c8ec8842ef1f95aa178ddc9506da0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Atgeirr=20Fl=C3=B8=20Rasmussen?= Date: Tue, 14 Apr 2015 15:29:55 +0200 Subject: [PATCH 5/8] Modified behaviour for unhandled arguments. Use constructor argument verify_syntax to decide course of action: if false, store unhandled arguments, if true, write a message and throw. --- opm/core/utility/parameters/ParameterGroup.hpp | 10 +++++++--- .../utility/parameters/ParameterGroup_impl.hpp | 14 +++++++++++--- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/opm/core/utility/parameters/ParameterGroup.hpp b/opm/core/utility/parameters/ParameterGroup.hpp index caecf4ad..1efd19a6 100644 --- a/opm/core/utility/parameters/ParameterGroup.hpp +++ b/opm/core/utility/parameters/ParameterGroup.hpp @@ -127,12 +127,16 @@ namespace Opm { /// /// It is required that argv[0] is the program name, while if /// 0 < i < argc, then argv[i] is either - /// the name of an xml file or parametername=value. + /// the name of an xml file, parameter file or parametername=value. /// /// \param argc is the number of command-line arguments, /// including the name of the executable. /// \param argv is an array of char*, each of which is a - /// command line argument. + /// command line argument. + /// \param verify_syntax If true (default), then it is an error to + /// pass arguments that cannot be handled by the ParameterGroup, + /// or an empty argument list. If false, such arguments are stored + /// and can be retrieved later with unhandledArguments(). template ParameterGroup(int argc, StringArray argv, const bool verify_syntax = true); @@ -284,7 +288,7 @@ namespace Opm { template T translate(const pair_type& data, const Requirement& chk) const; template - void parseCommandLineArguments(int argc, StringArray argv); + void parseCommandLineArguments(int argc, StringArray argv, bool verify_syntax); void recursiveSetIsOutputEnabled(bool output_is_enabled); // helper routines to do textual I/O diff --git a/opm/core/utility/parameters/ParameterGroup_impl.hpp b/opm/core/utility/parameters/ParameterGroup_impl.hpp index 96deee4a..232ba402 100644 --- a/opm/core/utility/parameters/ParameterGroup_impl.hpp +++ b/opm/core/utility/parameters/ParameterGroup_impl.hpp @@ -38,11 +38,13 @@ #include #include +#include #include #include #include #include +#include namespace Opm { namespace parameter { @@ -115,11 +117,11 @@ namespace Opm { << "[...]" << std::endl; exit(EXIT_FAILURE); } - this->parseCommandLineArguments(argc, argv); + this->parseCommandLineArguments(argc, argv, verify_syntax); } template - void ParameterGroup::parseCommandLineArguments(int argc, StringArray argv) + void ParameterGroup::parseCommandLineArguments(int argc, StringArray argv, bool verify_syntax) { std::vector files; std::vector > assignments; @@ -150,7 +152,13 @@ namespace Opm { } else if (file_type.second == "param") { this->readParam(files[i]); } else { - unhandled_arguments_.push_back(files[i]); + if (verify_syntax) { + std::cerr << "ERROR: Input '" << files[i] << "' is not a valid name for a parameter file.\n"; + std::cerr << " Valid filename extensions are 'xml' and 'param'.\n"; + OPM_THROW(std::runtime_error, "ParameterGroup cannot handle argument: " << files[i]); + } else { + unhandled_arguments_.push_back(files[i]); + } } } for (int i = 0; i < int(assignments.size()); ++i) { From 4381a8a9b3c87d23aa4f83c434cefb21be666a62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Atgeirr=20Fl=C3=B8=20Rasmussen?= Date: Tue, 14 Apr 2015 15:31:22 +0200 Subject: [PATCH 6/8] Update test to work correctly with new behaviour. Also add a test to verify that we throw when we are supposed to. --- tests/test_param.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/test_param.cpp b/tests/test_param.cpp index b9592dc7..9a04ef9a 100644 --- a/tests/test_param.cpp +++ b/tests/test_param.cpp @@ -73,6 +73,7 @@ BOOST_AUTO_TEST_CASE(commandline_syntax_init) "/slashtopitem=anotherstring\n" "/topitem=somestring\n"; BOOST_CHECK(os.str() == correct_answer); + BOOST_CHECK(p.unhandledArguments().empty()); // Tests that only run in debug mode. #ifndef NDEBUG @@ -88,7 +89,7 @@ BOOST_AUTO_TEST_CASE(xml_syntax_init) "/group/item=overridingstring", "unhandledargument" }; const std::size_t argc = sizeof(argv)/sizeof(argv[0]); - parameter::ParameterGroup p(argc, argv); + parameter::ParameterGroup p(argc, argv, false); BOOST_CHECK(p.get("topitem") == "somestring"); std::ostringstream os; p.writeParamToStream(os); @@ -105,3 +106,15 @@ BOOST_AUTO_TEST_CASE(xml_syntax_init) #ifndef NDEBUG #endif } + + +BOOST_AUTO_TEST_CASE(failing_strict_xml_syntax_init) +{ + typedef const char* cp; + cp argv[] = { "program_command", + "testdata.xml", + "/group/item=overridingstring", + "unhandledargument" }; + const std::size_t argc = sizeof(argv)/sizeof(argv[0]); + BOOST_CHECK_THROW(parameter::ParameterGroup p(argc, argv), std::runtime_error); +} From 37904a3e741cb32a5c1c21a74359535b6ff17e93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Atgeirr=20Fl=C3=B8=20Rasmussen?= Date: Tue, 14 Apr 2015 15:32:19 +0200 Subject: [PATCH 7/8] Make some programs stricter about parameter parsing. This applies to programs which always require at least one parameter. --- examples/compute_eikonal_from_files.cpp | 2 +- examples/compute_initial_state.cpp | 2 +- examples/compute_tof.cpp | 2 +- examples/compute_tof_from_files.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/examples/compute_eikonal_from_files.cpp b/examples/compute_eikonal_from_files.cpp index 8f25c25e..b583becb 100644 --- a/examples/compute_eikonal_from_files.cpp +++ b/examples/compute_eikonal_from_files.cpp @@ -60,7 +60,7 @@ try { using namespace Opm; - parameter::ParameterGroup param(argc, argv, false); + parameter::ParameterGroup param(argc, argv); // Read grid. GridManager grid_manager(param.get("grid_filename")); diff --git a/examples/compute_initial_state.cpp b/examples/compute_initial_state.cpp index 3b118c15..4eeec0b2 100644 --- a/examples/compute_initial_state.cpp +++ b/examples/compute_initial_state.cpp @@ -81,7 +81,7 @@ try using namespace Opm; // Setup. - parameter::ParameterGroup param(argc, argv, false); + parameter::ParameterGroup param(argc, argv); std::cout << "--------------- Reading parameters ---------------" << std::endl; const std::string deck_filename = param.get("deck_filename"); Opm::ParserPtr parser(new Opm::Parser() ); diff --git a/examples/compute_tof.cpp b/examples/compute_tof.cpp index 6945fa76..4d3148b5 100644 --- a/examples/compute_tof.cpp +++ b/examples/compute_tof.cpp @@ -160,7 +160,7 @@ try using namespace Opm; std::cout << "\n================ Test program for incompressible tof computations ===============\n\n"; - parameter::ParameterGroup param(argc, argv, false); + parameter::ParameterGroup param(argc, argv); std::cout << "--------------- Reading parameters ---------------" << std::endl; // Read the deck. diff --git a/examples/compute_tof_from_files.cpp b/examples/compute_tof_from_files.cpp index a029eb62..f0964aaa 100644 --- a/examples/compute_tof_from_files.cpp +++ b/examples/compute_tof_from_files.cpp @@ -77,7 +77,7 @@ try { using namespace Opm; - parameter::ParameterGroup param(argc, argv, false); + parameter::ParameterGroup param(argc, argv); // Read grid. GridManager grid_manager(param.get("grid_filename")); From d4e7ad3ea1a6be629405549e8ad1c0f5c5a00ee2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Atgeirr=20Fl=C3=B8=20Rasmussen?= Date: Tue, 14 Apr 2015 15:58:37 +0200 Subject: [PATCH 8/8] Make test argc/argv data mimic POSIX correct data. --- tests/test_param.cpp | 48 ++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/tests/test_param.cpp b/tests/test_param.cpp index 9a04ef9a..972b20db 100644 --- a/tests/test_param.cpp +++ b/tests/test_param.cpp @@ -47,22 +47,24 @@ #include #include #include +#include using namespace Opm; BOOST_AUTO_TEST_CASE(commandline_syntax_init) { typedef const char* cp; - cp argv[] = { "program_command", - "topitem=somestring", - "/slashtopitem=anotherstring", - "/group/item=1", - "/group/anotheritem=2", - "/group/subgroup/item=3", - "/group/subgroup/anotheritem=4", - "/group/item=overridingstring" }; - const std::size_t argc = sizeof(argv)/sizeof(argv[0]); - parameter::ParameterGroup p(argc, argv); + std::vector argv = { "program_command", + "topitem=somestring", + "/slashtopitem=anotherstring", + "/group/item=1", + "/group/anotheritem=2", + "/group/subgroup/item=3", + "/group/subgroup/anotheritem=4", + "/group/item=overridingstring", + 0 }; + const std::size_t argc = argv.size() - 1; + parameter::ParameterGroup p(argc, argv.data()); BOOST_CHECK(p.get("topitem") == "somestring"); std::ostringstream os; p.writeParamToStream(os); @@ -84,12 +86,13 @@ BOOST_AUTO_TEST_CASE(commandline_syntax_init) BOOST_AUTO_TEST_CASE(xml_syntax_init) { typedef const char* cp; - cp argv[] = { "program_command", - "testdata.xml", - "/group/item=overridingstring", - "unhandledargument" }; - const std::size_t argc = sizeof(argv)/sizeof(argv[0]); - parameter::ParameterGroup p(argc, argv, false); + std::vector argv = { "program_command", + "testdata.xml", + "/group/item=overridingstring", + "unhandledargument", + 0}; + const std::size_t argc = argv.size() - 1; + parameter::ParameterGroup p(argc, argv.data(), false); BOOST_CHECK(p.get("topitem") == "somestring"); std::ostringstream os; p.writeParamToStream(os); @@ -111,10 +114,11 @@ BOOST_AUTO_TEST_CASE(xml_syntax_init) BOOST_AUTO_TEST_CASE(failing_strict_xml_syntax_init) { typedef const char* cp; - cp argv[] = { "program_command", - "testdata.xml", - "/group/item=overridingstring", - "unhandledargument" }; - const std::size_t argc = sizeof(argv)/sizeof(argv[0]); - BOOST_CHECK_THROW(parameter::ParameterGroup p(argc, argv), std::runtime_error); + std::vector argv = { "program_command", + "testdata.xml", + "/group/item=overridingstring", + "unhandledargument", + 0 }; + const std::size_t argc = argv.size() - 1; + BOOST_CHECK_THROW(parameter::ParameterGroup p(argc, argv.data()), std::runtime_error); }