From 1f2775b7e242e3c126bf836e7e0de553b119cfd2 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Tue, 30 Jun 2020 10:46:24 -0600 Subject: [PATCH 1/8] clean up params error checking --- src/interface/params.hpp | 41 ++++++++++------------------------------ 1 file changed, 10 insertions(+), 31 deletions(-) diff --git a/src/interface/params.hpp b/src/interface/params.hpp index 279b033bf29a..c04c5b43f05f 100644 --- a/src/interface/params.hpp +++ b/src/interface/params.hpp @@ -17,12 +17,9 @@ #include #include #include -#include - -#ifndef NDEBUG #include #include -#endif // NDEBUG +#include #include "utils/error_checking.hpp" @@ -56,9 +53,11 @@ class Params { template const T &Get(const std::string key) { - keyCheck(key, false); - typeCheck(key, false); - auto typed_ptr = dynamic_cast *>(myParams_[key].get()); + auto it = myParams_.find(key); + PARTHENON_REQUIRE(it != myParams_.end(), + ("Key " + key + " doesn't exist").c_str()); + typeCheck(key); + auto typed_ptr = dynamic_cast *>((it->second).get()); if (typed_ptr == nullptr) throw std::invalid_argument("Cannot cast Params[" + key + "] to requested type"); return *typed_ptr->pValue; @@ -94,31 +93,11 @@ class Params { }; template - void typeCheck(const std::string key, bool die) { + void typeCheck(const std::string key) { // check on return type - // TODO(JMM): This is really clunky. Should we remove the conditional - // and put it in the require statement? - if (myTypes_[key].compare(std::string(typeid(T).name()))) { - std::string message = "WRONG TYPE FOR KEY '" + key + "'"; - PARTHENON_REQUIRE(!die, message.c_str()); - } - } - - void keyCheck(const std::string key, bool die) { -#ifndef NDEBUG - if (!hasKey(key)) { - // key alread exists, replace - std::cout << std::endl; - std::cout << "----------------------------------------------" << std::endl; - std::cout << " WARNING: key '" << key << "' not found." << std::endl; - std::cout << " Swift death will follow..." << std::endl; - std::cout << "----------------------------------------------" << std::endl; - std::cout << std::endl; - - std::string message = "Key " + key + " doesn't exist"; - PARTHENON_DEBUG_REQUIRE(!die, message.c_str()); - } -#endif // NDEBUG + bool badtype = myTypes_[key].compare(std::string(typeid(T).name())); + std::string message = "WRONG TYPE FOR KEY '" + key + "'"; + PARTHENON_REQUIRE(!badtype, message.c_str()); } std::map> myParams_; From 845a7e7684d342ad4d6657aeaab90c31e1199737 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Tue, 30 Jun 2020 10:48:00 -0600 Subject: [PATCH 2/8] clang format --- src/interface/params.hpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/interface/params.hpp b/src/interface/params.hpp index c04c5b43f05f..8053b965657b 100644 --- a/src/interface/params.hpp +++ b/src/interface/params.hpp @@ -54,8 +54,7 @@ class Params { template const T &Get(const std::string key) { auto it = myParams_.find(key); - PARTHENON_REQUIRE(it != myParams_.end(), - ("Key " + key + " doesn't exist").c_str()); + PARTHENON_REQUIRE(it != myParams_.end(), ("Key " + key + " doesn't exist").c_str()); typeCheck(key); auto typed_ptr = dynamic_cast *>((it->second).get()); if (typed_ptr == nullptr) From e3a348e214bdacac72c85e8cb55ea3e0c0b894ae Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Tue, 30 Jun 2020 11:10:47 -0600 Subject: [PATCH 3/8] format --- src/interface/params.hpp | 7 ++++++- tst/unit/test_unit_params.cpp | 22 +++++++--------------- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/interface/params.hpp b/src/interface/params.hpp index 8053b965657b..37ad98ded8ae 100644 --- a/src/interface/params.hpp +++ b/src/interface/params.hpp @@ -17,9 +17,12 @@ #include #include #include +#include + +#ifndef NDEBUG #include #include -#include +#endif // NDEBUG #include "utils/error_checking.hpp" @@ -94,9 +97,11 @@ class Params { template void typeCheck(const std::string key) { // check on return type +#ifndef NDEBUG bool badtype = myTypes_[key].compare(std::string(typeid(T).name())); std::string message = "WRONG TYPE FOR KEY '" + key + "'"; PARTHENON_REQUIRE(!badtype, message.c_str()); +#endif // NDEBUG } std::map> myParams_; diff --git a/tst/unit/test_unit_params.cpp b/tst/unit/test_unit_params.cpp index 8bc1dadf5c5d..78dc839c7be8 100644 --- a/tst/unit/test_unit_params.cpp +++ b/tst/unit/test_unit_params.cpp @@ -39,41 +39,33 @@ TEST_CASE("Add and Get is called", "[Add,Get]") { REQUIRE_THROWS_AS(params.Get(key), std::invalid_argument); } } - - GIVEN("An empty params structure") { - Params params; - WHEN(" attempting to get a key that does not exist ") { - std::string non_existent_key = "key"; - REQUIRE_THROWS_AS(params.Get(non_existent_key), std::invalid_argument); - } - } } -TEST_CASE("reset is called", "[reset]") { +TEST_CASE("when hasKey is called", "[hasKey]") { GIVEN("A key is added") { Params params; std::string key = "test_key"; double value = -2.0; params.Add(key, value); + + REQUIRE(params.hasKey(key) == true); + WHEN("the params are reset") { params.reset(); - REQUIRE_THROWS_AS(params.Get(key), std::invalid_argument); + REQUIRE(params.hasKey(key) == false); } } } -TEST_CASE("when hasKey is called", "[hasKey]") { +TEST_CASE("reset is called", "[reset]") { GIVEN("A key is added") { Params params; std::string key = "test_key"; double value = -2.0; params.Add(key, value); - - REQUIRE(params.hasKey(key) == true); - WHEN("the params are reset") { params.reset(); - REQUIRE(params.hasKey(key) == false); + REQUIRE(!params.hasKey(key)); } } } From 8319308ccbc99af550c13873c8a326afc3e5f1c8 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Sat, 4 Jul 2020 14:02:57 -0600 Subject: [PATCH 4/8] rely on type check and parthenon_require for params typing --- src/interface/params.hpp | 9 +-------- tst/unit/test_unit_params.cpp | 4 ---- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/src/interface/params.hpp b/src/interface/params.hpp index 37ad98ded8ae..cb920d741762 100644 --- a/src/interface/params.hpp +++ b/src/interface/params.hpp @@ -17,12 +17,9 @@ #include #include #include -#include - -#ifndef NDEBUG #include #include -#endif // NDEBUG +#include #include "utils/error_checking.hpp" @@ -60,8 +57,6 @@ class Params { PARTHENON_REQUIRE(it != myParams_.end(), ("Key " + key + " doesn't exist").c_str()); typeCheck(key); auto typed_ptr = dynamic_cast *>((it->second).get()); - if (typed_ptr == nullptr) - throw std::invalid_argument("Cannot cast Params[" + key + "] to requested type"); return *typed_ptr->pValue; } @@ -97,11 +92,9 @@ class Params { template void typeCheck(const std::string key) { // check on return type -#ifndef NDEBUG bool badtype = myTypes_[key].compare(std::string(typeid(T).name())); std::string message = "WRONG TYPE FOR KEY '" + key + "'"; PARTHENON_REQUIRE(!badtype, message.c_str()); -#endif // NDEBUG } std::map> myParams_; diff --git a/tst/unit/test_unit_params.cpp b/tst/unit/test_unit_params.cpp index 78dc839c7be8..43c3b0b480ee 100644 --- a/tst/unit/test_unit_params.cpp +++ b/tst/unit/test_unit_params.cpp @@ -34,10 +34,6 @@ TEST_CASE("Add and Get is called", "[Add,Get]") { WHEN("the same key is provided a second time") { REQUIRE_THROWS_AS(params.Add(key, value), std::invalid_argument); } - - WHEN("attempting to get the key but casting to a different type") { - REQUIRE_THROWS_AS(params.Get(key), std::invalid_argument); - } } } From 9de487cee9d8db9bf418a59b107e15d4110efeca Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Mon, 6 Jul 2020 13:17:44 -0600 Subject: [PATCH 5/8] add error throwing to error checking --- docs/README.md | 18 ++++++--- src/interface/params.hpp | 17 ++------ src/utils/error_checking.hpp | 73 +++++++++++++++++++++++++++++++++++ tst/unit/test_unit_params.cpp | 28 ++++++++++---- 4 files changed, 109 insertions(+), 27 deletions(-) diff --git a/docs/README.md b/docs/README.md index 66c182dea415..3f76008e24f6 100644 --- a/docs/README.md +++ b/docs/README.md @@ -53,15 +53,21 @@ There are several weakly linked member functions that applications can (and ofte Macros for causing execution to throw an exception are provided [here](../src/utils/error_checking.hpp) * PARTHENON_REQUIRE(condition, message) exits if the condition does not evaluate to true. +* PARTHENON_REQUIRE_THROWS(condition, message) throws a `std::runtime_error` exception if the condition does not evaluate to true. * PARTHENON_FAIL(message) always exits. +* PARTHENON_THROW(message) throws a runtime error. * PARTHENON_DEBUG_REQUIRE(condition, message) exits if the condition does not evaluate to true when in debug mode. +* PARTHENON_DEBUG_REQUIRE_THROWS(condition, message) throws if the condition does not evaluate to true when in debug mode. * PARTHENON_DEBUG_FAIL(message) always exits when in debug mode. - -Both macros print the message, and filename and line number where the -macro is called. PARTHENON_REQUIRE also prints the condition. Note -that these macros take a C style string, not a C++ style string. This -is a limitation of GPU compatibility. Examples of use can be found -[here](../tst/unit/test_error_checking.cpp). +* PARTHENON_DEBUG_THROW(message) throws a runtime error when in debug mode. + +All macros print the message, and filename and line number where the +macro is called. PARTHENON_REQUIRE also prints the condition. The +macros take a `std::string`, a `std::stringstream`, or a C-style +string. As a rule of thumb: +- Use the exception throwing versions in non-GPU, non-performance critical code. +- On GPUs and in performance-critical sections, use the non-throwing + versions and give them C-style strings. ### Developer guide diff --git a/src/interface/params.hpp b/src/interface/params.hpp index cb920d741762..82f492876000 100644 --- a/src/interface/params.hpp +++ b/src/interface/params.hpp @@ -39,9 +39,7 @@ class Params { /// Throws an error if the key is already in use template void Add(const std::string &key, T value) { - if (hasKey(key)) { - throw std::invalid_argument("Key value pair already exists, cannot add key."); - } + PARTHENON_REQUIRE_THROWS(!(hasKey(key)), "Key " + key + "already exists"); myParams_[key] = std::unique_ptr(new object_t(value)); myTypes_[key] = std::string(typeid(value).name()); } @@ -54,8 +52,9 @@ class Params { template const T &Get(const std::string key) { auto it = myParams_.find(key); - PARTHENON_REQUIRE(it != myParams_.end(), ("Key " + key + " doesn't exist").c_str()); - typeCheck(key); + PARTHENON_REQUIRE_THROWS(it != myParams_.end(), "Key " + key + " doesn't exist"); + PARTHENON_REQUIRE_THROWS(!(myTypes_[key].compare(std::string(typeid(T).name()))), + "WRONG TYPE FOR KEY '" + key + "'"); auto typed_ptr = dynamic_cast *>((it->second).get()); return *typed_ptr->pValue; } @@ -89,14 +88,6 @@ class Params { const void *address() { return reinterpret_cast(pValue.get()); } }; - template - void typeCheck(const std::string key) { - // check on return type - bool badtype = myTypes_[key].compare(std::string(typeid(T).name())); - std::string message = "WRONG TYPE FOR KEY '" + key + "'"; - PARTHENON_REQUIRE(!badtype, message.c_str()); - } - std::map> myParams_; std::map myTypes_; }; diff --git a/src/utils/error_checking.hpp b/src/utils/error_checking.hpp index 4d7e53538a0f..e44e45c7a291 100644 --- a/src/utils/error_checking.hpp +++ b/src/utils/error_checking.hpp @@ -20,6 +20,8 @@ // \brief utility macros for error checking #include +#include +#include #include @@ -28,21 +30,42 @@ parthenon::ErrorChecking::require(#condition, message, __FILE__, __LINE__); \ } +#define PARTHENON_REQUIRE_THROWS(condition, message) \ + if (!(condition)) { \ + parthenon::ErrorChecking::require_throws(#condition, message, __FILE__, __LINE__); \ + } + #define PARTHENON_FAIL(message) \ parthenon::ErrorChecking::fail(message, __FILE__, __LINE__); +#define PARTHENON_THROW(message) \ + parthenon::ErrorChecking::fail_throws(message, __FILE__, __LINE__); + #ifdef NDEBUG #define PARTHENON_DEBUG_REQUIRE(condition, message) ((void)0) #else #define PARTHENON_DEBUG_REQUIRE(condition, message) PARTHENON_REQUIRE(condition, message) #endif +#ifdef NDEBUG +#define PARTHENON_DEBUG_REQUIRE_THROWS(condition, message) ((void)0) +#else +#define PARTHENON_DEBUG_REQUIRE_THROWS(condition, message) \ + PARTHENON_REQUIRE_THROWS(condition, message) +#endif + #ifdef NDEBUG #define PARTHENON_DEBUG_FAIL(message) ((void)0) #else #define PARTHENON_DEBUG_FAIL(message) PARTHENON_FAIL(message) #endif +#ifdef NDEBUG +#define PARTHENON_DEBUG_THROW(message) ((void)0) +#else +#define PARTHENON_DEBUG_THROW(message) PARTHENON_THROW(message) +#endif + namespace parthenon { namespace ErrorChecking { @@ -55,6 +78,37 @@ void require(const char *const condition, const char *const message, Kokkos::abort(message); } +inline void require(const char *const condition, std::string const &message, + const char *const filename, int const linenumber) { + require(condition, message.c_str(), filename, linenumber); +} + +inline void require(const char *const condition, std::stringstream const &message, + const char *const filename, int const linenumber) { + require(condition, message.str().c_str(), filename, linenumber); +} + +// TODO(JMM): should we define our own parthenon error class? Or is +// std::runtime_error good enough? +inline void require_throws(const char *const condition, const char *const message, + const char *const filename, int const linenumber) { + std::stringstream msg; + msg << "### PARTHENON ERROR\n Condition: " << condition + << "\n Message: " << message << "\n File: " << filename + << "\n Line number: " << linenumber << std::endl; + throw std::runtime_error(msg.str().c_str()); +} + +inline void require_throws(const char *const condition, std::string const &message, + const char *const filename, int const linenumber) { + require_throws(condition, message.c_str(), filename, linenumber); +} + +inline void require_throws(const char *const condition, std::stringstream const &message, + const char *const filename, int const linenumber) { + require_throws(condition, message.str().c_str(), filename, linenumber); +} + KOKKOS_INLINE_FUNCTION void fail(const char *const message, const char *const filename, int const linenumber) { printf("### PARTHENON ERROR\n Message: %s\n File: %s\n Line number: %i\n", @@ -67,6 +121,25 @@ inline void fail(std::stringstream const &message, const char *const filename, fail(message.str().c_str(), filename, linenumber); } +inline void fail_throws(const char *const message, const char *const filename, + int const linenumber) { + std::stringstream msg; + msg << "### PARTHENON ERROR\n Message: " << message + << "\n File: " << filename << "\n Line number: " << linenumber + << std::endl; + throw std::runtime_error(msg.str().c_str()); +} + +inline void fail_throws(std::string const &message, const char *const filename, + int const linenumber) { + fail_throws(message.c_str(), filename, linenumber); +} + +inline void fail_throws(std::stringstream const &message, const char *const filename, + int const linenumber) { + fail_throws(message.str().c_str(), filename, linenumber); +} + } // namespace ErrorChecking } // namespace parthenon diff --git a/tst/unit/test_unit_params.cpp b/tst/unit/test_unit_params.cpp index 43c3b0b480ee..fb61e8fe469c 100644 --- a/tst/unit/test_unit_params.cpp +++ b/tst/unit/test_unit_params.cpp @@ -32,36 +32,48 @@ TEST_CASE("Add and Get is called", "[Add,Get]") { double output = params.Get(key); REQUIRE(output == Approx(value)); WHEN("the same key is provided a second time") { - REQUIRE_THROWS_AS(params.Add(key, value), std::invalid_argument); + REQUIRE_THROWS_AS(params.Add(key, value), std::runtime_error); + } + + WHEN("attempting to get the key but casting to a different type") { + REQUIRE_THROWS_AS(params.Get(key), std::runtime_error); + } + } + + GIVEN("An empty params structure") { + Params params; + WHEN(" attempting to get a key that does not exist ") { + std::string non_existent_key = "key"; + REQUIRE_THROWS_AS(params.Get(non_existent_key), std::runtime_error); } } } -TEST_CASE("when hasKey is called", "[hasKey]") { +TEST_CASE("reset is called", "[reset]") { GIVEN("A key is added") { Params params; std::string key = "test_key"; double value = -2.0; params.Add(key, value); - - REQUIRE(params.hasKey(key) == true); - WHEN("the params are reset") { params.reset(); - REQUIRE(params.hasKey(key) == false); + REQUIRE_THROWS_AS(params.Get(key), std::runtime_error); } } } -TEST_CASE("reset is called", "[reset]") { +TEST_CASE("when hasKey is called", "[hasKey]") { GIVEN("A key is added") { Params params; std::string key = "test_key"; double value = -2.0; params.Add(key, value); + + REQUIRE(params.hasKey(key) == true); + WHEN("the params are reset") { params.reset(); - REQUIRE(!params.hasKey(key)); + REQUIRE(params.hasKey(key) == false); } } } From 478e1b709d8a1c168076d6731a0a54952257930b Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Tue, 21 Jul 2020 10:46:06 -0600 Subject: [PATCH 6/8] fix lint and format issues --- src/utils/error_checking.hpp | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/utils/error_checking.hpp b/src/utils/error_checking.hpp index 7f9ef8130035..ed86fefdafd0 100644 --- a/src/utils/error_checking.hpp +++ b/src/utils/error_checking.hpp @@ -148,19 +148,18 @@ inline void fail_throws(std::stringstream const &message, const char *const file int const linenumber) { fail_throws(message.str().c_str(), filename, linenumber); -KOKKOS_INLINE_FUNCTION -void warn(const char *const message, const char *const filename, int const linenumber) { - printf( - "### PARTHENON WARNING\n Message: %s\n File: %s\n Line number: %i\n", - message, filename, linenumber); -} - -inline void warn(std::stringstream const &message, const char *const filename, - int const linenumber) { - warn(message.str().c_str(), filename, linenumber); -} + KOKKOS_INLINE_FUNCTION + void warn(const char *const message, const char *const filename, int const linenumber) { + printf("### PARTHENON WARNING\n Message: %s\n File: %s\n Line number: " + "%i\n", + message, filename, linenumber); + } + inline void warn(std::stringstream const &message, const char *const filename, + int const linenumber) { + warn(message.str().c_str(), filename, linenumber); + } +} // namespace ErrorChecking } // namespace ErrorChecking -} // namespace parthenon #endif // UTILS_ERROR_CHECKING_HPP_ From 2b5a5dc3a9d8ac9788000d915d5266d6cfa3be65 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Tue, 21 Jul 2020 18:32:12 -0600 Subject: [PATCH 7/8] fixed bad merge typo in error checking --- src/utils/error_checking.hpp | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/utils/error_checking.hpp b/src/utils/error_checking.hpp index ed86fefdafd0..62bd580827ad 100644 --- a/src/utils/error_checking.hpp +++ b/src/utils/error_checking.hpp @@ -147,19 +147,19 @@ inline void fail_throws(std::string const &message, const char *const filename, inline void fail_throws(std::stringstream const &message, const char *const filename, int const linenumber) { fail_throws(message.str().c_str(), filename, linenumber); +} - KOKKOS_INLINE_FUNCTION - void warn(const char *const message, const char *const filename, int const linenumber) { - printf("### PARTHENON WARNING\n Message: %s\n File: %s\n Line number: " - "%i\n", - message, filename, linenumber); - } +KOKKOS_INLINE_FUNCTION +void warn(const char *const message, const char *const filename, int const linenumber) { + printf("### PARTHENON WARNING\n Message: %s\n File: %s\n Line number: " + "%i\n", + message, filename, linenumber); +} - inline void warn(std::stringstream const &message, const char *const filename, - int const linenumber) { - warn(message.str().c_str(), filename, linenumber); - } -} // namespace ErrorChecking +inline void warn(std::stringstream const &message, const char *const filename, + int const linenumber) { + warn(message.str().c_str(), filename, linenumber); +} } // namespace ErrorChecking #endif // UTILS_ERROR_CHECKING_HPP_ From 14308c6540ee4ffdeebe019f76cdea0cadb277d6 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Tue, 21 Jul 2020 18:46:28 -0600 Subject: [PATCH 8/8] missing curly brace from bad merge --- src/utils/error_checking.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/utils/error_checking.hpp b/src/utils/error_checking.hpp index 62bd580827ad..8204563d80b6 100644 --- a/src/utils/error_checking.hpp +++ b/src/utils/error_checking.hpp @@ -161,5 +161,6 @@ inline void warn(std::stringstream const &message, const char *const filename, warn(message.str().c_str(), filename, linenumber); } } // namespace ErrorChecking +} // namespace parthenon #endif // UTILS_ERROR_CHECKING_HPP_