Merge pull request #210 from lanl/jmm/cleanup-params-error-checking

Clean up error checking in params

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
 

src/interface/params.hpp

@@ -17,12 +17,9 @@
 #include <map>
 #include <memory>
 #include <string>
-#include <vector>
-
-#ifndef NDEBUG
 #include <typeindex>
 #include <typeinfo>
-#endif // NDEBUG
+#include <vector>
 
 #include "utils/error_checking.hpp"
 
@@ -42,9 +39,7 @@ class Params {
   /// Throws an error if the key is already in use
   template <typename T>
   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<Params::base_t>(new object_t<T>(value));
     myTypes_[key] = std::string(typeid(value).name());
   }
@@ -56,11 +51,11 @@ class Params {
 
   template <typename T>
   const T &Get(const std::string key) {
-    keyCheck(key, false);
-    typeCheck<T>(key, false);
-    auto typed_ptr = dynamic_cast<Params::object_t<T> *>(myParams_[key].get());
-    if (typed_ptr == nullptr)
-      throw std::invalid_argument("Cannot cast Params[" + key + "] to requested type");
+    auto it = myParams_.find(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<Params::object_t<T> *>((it->second).get());
     return *typed_ptr->pValue;
   }
 
@@ -93,34 +88,6 @@ class Params {
     const void *address() { return reinterpret_cast<void *>(pValue.get()); }
   };
 
-  template <typename T>
-  void typeCheck(const std::string key, bool die) {
-    // 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
-  }
-
   std::map<std::string, std::unique_ptr<Params::base_t>> myParams_;
   std::map<std::string, std::string> myTypes_;
 };

src/utils/error_checking.hpp

@@ -20,6 +20,8 @@
 //  \brief utility macros for error checking
 
 #include <iostream>
+#include <stdexcept>
+#include <string>
 
 #include <Kokkos_Core.hpp>
 
@@ -28,9 +30,17 @@
     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__);
+
 #define PARTHENON_WARN(message)                                                          \
   parthenon::ErrorChecking::warn(message, __FILE__, __LINE__);
 
@@ -40,12 +50,25 @@
 #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
+
 #ifdef NDEBUG
 #define PARTHENON_DEBUG_WARN(message) ((void)0)
 #else
@@ -64,6 +87,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",
@@ -76,18 +130,36 @@ 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);
+}
+
 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);
+  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 parthenon
 

tst/unit/test_unit_params.cpp

@@ -32,19 +32,19 @@ TEST_CASE("Add and Get is called", "[Add,Get][coverage]") {
     double output = params.Get<double>(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<int>(key), std::invalid_argument);
+      REQUIRE_THROWS_AS(params.Get<int>(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<double>(non_existent_key), std::invalid_argument);
+      REQUIRE_THROWS_AS(params.Get<double>(non_existent_key), std::runtime_error);
     }
   }
 }
@@ -57,7 +57,7 @@ TEST_CASE("reset is called", "[reset][coverage]") {
     params.Add(key, value);
     WHEN("the params are reset") {
       params.reset();
-      REQUIRE_THROWS_AS(params.Get<double>(key), std::invalid_argument);
+      REQUIRE_THROWS_AS(params.Get<double>(key), std::runtime_error);
     }
   }
 }