From 21c4d292fed4268f340db9154cfef279f9f94924 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 22 Jan 2026 09:55:14 -0500 Subject: [PATCH 01/20] Shift definition GrackleCtxPack to its own file --- tests/grtestutils/CMakeLists.txt | 1 + tests/grtestutils/GrackleCtxPack.hpp | 100 ++++++++++++++++++++++ tests/grtestutils/googletest/fixtures.hpp | 3 +- tests/grtestutils/iterator_adaptor.hpp | 2 +- tests/grtestutils/preset.hpp | 80 ----------------- 5 files changed, 104 insertions(+), 82 deletions(-) create mode 100644 tests/grtestutils/GrackleCtxPack.hpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index 27bf11d17..3915849f9 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -80,6 +80,7 @@ add_library(grtestutils # files outside of the googletest subdirectory # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp + GrackleCtxPack.hpp iterator_adaptor.hpp os.hpp os.cpp preset.hpp preset.cpp diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp new file mode 100644 index 000000000..bce9abbca --- /dev/null +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -0,0 +1,100 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Declare the GrackleCtxType +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_GRACKLECTXPACK_HPP +#define GRTESTUTILS_GRACKLECTXPACK_HPP + +#include "preset.hpp" + +namespace grtest { +/// Tracks the group of Grackle objects needed for executing API functions +/// +/// The primary motivation for this object's existence is making sure that +/// the allocations get cleaned up when a test fails +/// +/// @note +/// Ideally, we would only make it possible to create a fully initialized +/// instance (in that case, we would delete the default constructor), but that +/// involves a bunch more work +/// +/// We choose to implement this in terms of std::unique_ptr (rather than raw +/// pointers) since it implements proper move semantics for us +class GrackleCtxPack { + /// units used for initializing chemistry_data + code_units initial_units_; + /// the fully initialized chemistry_data instance + std::unique_ptr my_chemistry_; + /// the fully initialized chemistry_data_storage_instance + std::unique_ptr my_rates_; + +public: + /// Construct an uninitialized instance + GrackleCtxPack() : my_chemistry_(nullptr), my_rates_(nullptr) {} + + GrackleCtxPack(GrackleCtxPack&&) = default; + GrackleCtxPack& operator=(GrackleCtxPack&&) = default; + + // forbid copy and assignment operations... + // -> we could re-enable them if we wanted to be able add to clone the + // types (or if we wanted to internally use std::shared_ptr + GrackleCtxPack(const GrackleCtxPack&) = delete; + GrackleCtxPack& operator=(const GrackleCtxPack&) = delete; + + ~GrackleCtxPack() { + if (!this->is_initialized()) { + return; + } + local_free_chemistry_data(this->my_chemistry_.get(), this->my_rates_.get()); + // unique_ptr destructor will handle calls to delete + } + + bool is_initialized() const { return this->my_chemistry_ != nullptr; } + + // getter functions + const code_units& initial_units() const { return this->initial_units_; } + chemistry_data* my_chemistry() { return this->my_chemistry_.get(); } + chemistry_data_storage* my_rates() { return this->my_rates_.get(); } + + /// create an initialized instance from a preset + static GrackleCtxPack create(const FullConfPreset& preset, + InitStatus* status) { + std::unique_ptr my_chemistry(new chemistry_data); + InitStatus tmp = + setup_chemistry_data_from_preset(my_chemistry.get(), preset.chemistry); + if (tmp != InitStatus::success) { + if (status != nullptr) { + *status = tmp; + } + return GrackleCtxPack(); // return an unitialized instance + } + + code_units initial_unit = setup_initial_unit(preset.unit); + std::unique_ptr my_rates( + new chemistry_data_storage); + if (local_initialize_chemistry_data(my_chemistry.get(), my_rates.get(), + &initial_unit) != GR_SUCCESS) { + if (status != nullptr) { + *status = InitStatus::generic_fail; + } + return GrackleCtxPack(); // return an unitialized instance + } + + GrackleCtxPack out; + out.initial_units_ = initial_unit; + out.my_chemistry_ = std::move(my_chemistry); + out.my_rates_ = std::move(my_rates); + return out; + } +}; + +} // namespace grtest + +#endif // GRTESTUTILS_GRACKLECTXPACK_HPP diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index 05f1ccb97..fc72c4a3a 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -6,7 +6,7 @@ //===----------------------------------------------------------------------===// /// /// @file -/// Define machinery for creating GoogleTest Fixutures to help test Grackle's +/// Define machinery for creating GoogleTest Fixtures to help test Grackle's /// C API. /// //===----------------------------------------------------------------------===// @@ -18,6 +18,7 @@ // because we include gtest.h here, we should NOT include this file in any // grtest source files (in other words, this should be a header-only file) +#include "../GrackleCtxPack.hpp" #include "../preset.hpp" namespace grtest { diff --git a/tests/grtestutils/iterator_adaptor.hpp b/tests/grtestutils/iterator_adaptor.hpp index ad84a3559..c992b9490 100644 --- a/tests/grtestutils/iterator_adaptor.hpp +++ b/tests/grtestutils/iterator_adaptor.hpp @@ -16,7 +16,7 @@ #include #include "grackle.h" -#include "preset.hpp" +#include "GrackleCtxPack.hpp" namespace grtest { diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index 919429a09..b1278d175 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -63,86 +63,6 @@ struct FullConfPreset { // teach googletest how to print FullConfPreset void PrintTo(const FullConfPreset& preset, std::ostream* os); -/// Tracks the group of Grackle objects needed for executing API functions -/// -/// The primary motivation for this object's existence is making sure that -/// the allocations get cleaned up when a test fails -/// -/// @note -/// Ideally, we would only make it possible to create a fully initialized -/// instance (in that case, we would delete the default constructor), but that -/// involves a bunch more work -/// -/// We choose to implement this in terms of std::unique_ptr (rather than raw -/// pointers) since it implements proper move semantics for us -class GrackleCtxPack { - /// units used for initializing chemistry_data - code_units initial_units_; - /// the fully initialized chemistry_data instance - std::unique_ptr my_chemistry_; - /// the fully initialized chemistry_data_storage_instance - std::unique_ptr my_rates_; - -public: - /// Construct an uninitialized instance - GrackleCtxPack() : my_chemistry_(nullptr), my_rates_(nullptr) {} - - GrackleCtxPack(GrackleCtxPack&&) = default; - GrackleCtxPack& operator=(GrackleCtxPack&&) = default; - - // forbid copy and assignment operations... - // -> we could re-enable them if we wanted to be able add to clone the - // types (or if we wanted to internally use std::shared_ptr - GrackleCtxPack(const GrackleCtxPack&) = delete; - GrackleCtxPack& operator=(const GrackleCtxPack&) = delete; - - ~GrackleCtxPack() { - if (!this->is_initialized()) { - return; - } - local_free_chemistry_data(this->my_chemistry_.get(), this->my_rates_.get()); - // unique_ptr destructor will handle calls to delete - } - - bool is_initialized() const { return this->my_chemistry_ != nullptr; } - - // getter functions - const code_units& initial_units() const { return this->initial_units_; } - chemistry_data* my_chemistry() { return this->my_chemistry_.get(); } - chemistry_data_storage* my_rates() { return this->my_rates_.get(); } - - /// create an initialized instance from a preset - static GrackleCtxPack create(const FullConfPreset& preset, - InitStatus* status) { - std::unique_ptr my_chemistry(new chemistry_data); - InitStatus tmp = - setup_chemistry_data_from_preset(my_chemistry.get(), preset.chemistry); - if (tmp != InitStatus::success) { - if (status != nullptr) { - *status = tmp; - } - return GrackleCtxPack(); // return an unitialized instance - } - - code_units initial_unit = setup_initial_unit(preset.unit); - std::unique_ptr my_rates( - new chemistry_data_storage); - if (local_initialize_chemistry_data(my_chemistry.get(), my_rates.get(), - &initial_unit) != GR_SUCCESS) { - if (status != nullptr) { - *status = InitStatus::generic_fail; - } - return GrackleCtxPack(); // return an unitialized instance - } - - GrackleCtxPack out; - out.initial_units_ = initial_unit; - out.my_chemistry_ = std::move(my_chemistry); - out.my_rates_ = std::move(my_rates); - return out; - } -}; - } // namespace grtest #endif // GRTESTUTILS_PRESET_HPP From 2da8d074ddfee8d03d70a7c4f4085217a9762c2a Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 22 Jan 2026 13:28:10 -0500 Subject: [PATCH 02/20] add wrappers to set chemistry_data params --- tests/grtestutils/CMakeLists.txt | 2 +- tests/grtestutils/GrackleCtxPack.cpp | 58 +++++++++++++++++++ tests/grtestutils/GrackleCtxPack.hpp | 86 ++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 tests/grtestutils/GrackleCtxPack.cpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index 3915849f9..586f164e1 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -80,7 +80,7 @@ add_library(grtestutils # files outside of the googletest subdirectory # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp - GrackleCtxPack.hpp + GrackleCtxPack.hpp GrackleCtxPack.cpp iterator_adaptor.hpp os.hpp os.cpp preset.hpp preset.cpp diff --git a/tests/grtestutils/GrackleCtxPack.cpp b/tests/grtestutils/GrackleCtxPack.cpp new file mode 100644 index 000000000..beed5be30 --- /dev/null +++ b/tests/grtestutils/GrackleCtxPack.cpp @@ -0,0 +1,58 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Implement logic pertaining to GrackleCtxPack +/// +//===----------------------------------------------------------------------===// + +#include "GrackleCtxPack.hpp" +#include "status_reporting.h" + +#include + +namespace grtest::param_detail { + +static bool set_str_(chemistry_data& my_chem, const std::string& name, + const char* val, std::size_t sz_with_nul, + StrAllocTracker* str_allocs) { + // NOTE: we should NOT directly modify characters held by field_ptr + const char** dest = const_cast( + local_chemistry_data_access_string(&my_chem, name.c_str())); + if (dest == nullptr) { + return false; // field is either not known or not a string + } + + // the following acts as a compiler hint (to suppress a warning) + GR_INTERNAL_REQUIRE((val == nullptr) != (sz_with_nul == 0), "compiler-hint"); + + if (str_allocs != nullptr) { + // (if applicable) allocate a new buffer and deallocate the old buffer + char* new_alloc = str_allocs->alloc_buf_and_free_old(sz_with_nul, *dest); + if (sz_with_nul > 0) { + std::memcpy(new_alloc, val, sz_with_nul - 1); + new_alloc[sz_with_nul - 1] = '\0'; + } + (*dest) = new_alloc; + } else { + (*dest) = val; + } + return true; +} + +bool set_str(chemistry_data& my_chem, const std::string& name, const char* val, + StrAllocTracker* str_allocs) { + std::size_t sz_with_nul = (val == nullptr) ? 0 : std::strlen(val) + 1; + return set_str_(my_chem, name, val, sz_with_nul, str_allocs); +} + +bool set_str(chemistry_data& my_chem, const std::string& name, + const std::string val, StrAllocTracker* str_allocs) { + return set_str_(my_chem, name, val.data(), val.size() + 1, str_allocs); +} + +} // namespace grtest::param_detail \ No newline at end of file diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index bce9abbca..db8b58f39 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -13,8 +13,94 @@ #define GRTESTUTILS_GRACKLECTXPACK_HPP #include "preset.hpp" +#include // std::remove_if +#include // std::unique_ptr namespace grtest { + +namespace param_detail { + +/// Tracks allocations of string parameters stored by a @ref chemistry_data +/// +/// The basic premise is that when you construct a @ref chemistry_data instance, +/// you would also construct a StrAllocTracker instance that has the same +/// lifetime as @ref chemistry_data. And then you would pass both instances to +/// @ref set_str when you want to store a string parameter +/// +/// @par Implementation Notes +/// * We explicitly avoid tracking the allocations within this type using +/// std::string because SSO (small string optimization) could cause weird +/// bugs in the future during refactoring if we aren't super careful +/// +/// * We assume max number of strings is small (i.e. <=5). If the number grows, +/// use std::unordered_set instead of std::vector +class StrAllocTracker { + /// manages the lifetime of string allocations + std::vector> bufs_; + +public: + /// allocate a new buffer and try to free the old buffer + /// + /// @param tot_len Total length of new buffer (including the nul terminator) + /// @param old The old buffer that we are trying to replace + char* alloc_buf_and_free_old(std::size_t tot_len, const char* old) { + // erase the entry in bufs_ holding old (this also frees memory). If old + // isn't found, it's a nullptr or we assume that it's a string literal + for (std::size_t i = 0; i < bufs_.size(); i++) { + if (bufs_[i].get() == old) { + bufs_.erase(bufs_.begin() + i); + break; + } + } + + if (tot_len == 0) { + return nullptr; + } + const std::unique_ptr& p = bufs_.emplace_back(new char[tot_len]); + return p.get(); + } +}; + +/// Tries to set a string parameter tracked by @p my_param +/// +/// @param[in,out] my_chem Tracks various Grackle parameters +/// @param[in] name The name of the parameter getting updated +/// @param[in] val The value of the parameter +/// @param[in] str_allocs Tracks allocations of the strings held by @p my_chem +/// @returns true if successful and `false` if there was an error (e.g. @p name +/// isn't a known parameter or is a parameter that doesn't expect a string) +/// +/// @note +/// Oridinarily, this function will update @p str_allocs to hold a copy of +/// @p val and @p my_chem will be updated to store a pointer to that copy. When +/// @p str_allocs is a nullptr, @p my_chem is directly updated to track the data +/// referenced by @p val. +bool set_str(chemistry_data& my_chem, const std::string& name, const char* val, + StrAllocTracker* str_allocs); +bool set_str(chemistry_data& my_chem, const std::string& name, + const std::string& val, StrAllocTracker* str_allocs); + +inline bool set_int(chemistry_data& my_chem, const std::string& name, int val) { + int* dest = local_chemistry_data_access_int(&my_chem, name.c_str()); + if (dest == nullptr) { + return false; + } + (*dest) = val; + return true; +} + +inline bool set_double(chemistry_data& my_chem, const std::string& name, + double val) { + double* dest = local_chemistry_data_access_double(&my_chem, name.c_str()); + if (dest == nullptr) { + return false; + } + (*dest) = val; + return true; +} + +} // namespace param_detail + /// Tracks the group of Grackle objects needed for executing API functions /// /// The primary motivation for this object's existence is making sure that From 774b3057d9e1875af682c3d56601db0435097f30 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 22 Jan 2026 15:11:29 -0500 Subject: [PATCH 03/20] grtestutils: chemistry_data parameter machinery --- tests/grtestutils/CMakeLists.txt | 3 +- tests/grtestutils/GrackleCtxPack.cpp | 58 --------- tests/grtestutils/GrackleCtxPack.hpp | 37 ------ tests/grtestutils/param.cpp | 179 +++++++++++++++++++++++++++ tests/grtestutils/param.hpp | 171 +++++++++++++++++++++++++ 5 files changed, 352 insertions(+), 96 deletions(-) delete mode 100644 tests/grtestutils/GrackleCtxPack.cpp create mode 100644 tests/grtestutils/param.cpp create mode 100644 tests/grtestutils/param.hpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index 586f164e1..bf12b77f4 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -80,9 +80,10 @@ add_library(grtestutils # files outside of the googletest subdirectory # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp - GrackleCtxPack.hpp GrackleCtxPack.cpp + GrackleCtxPack.hpp iterator_adaptor.hpp os.hpp os.cpp + param.hpp param.cpp preset.hpp preset.cpp utils.hpp utils.cpp diff --git a/tests/grtestutils/GrackleCtxPack.cpp b/tests/grtestutils/GrackleCtxPack.cpp deleted file mode 100644 index beed5be30..000000000 --- a/tests/grtestutils/GrackleCtxPack.cpp +++ /dev/null @@ -1,58 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// See the LICENSE file for license and copyright information -// SPDX-License-Identifier: NCSA AND BSD-3-Clause -// -//===----------------------------------------------------------------------===// -/// -/// @file -/// Implement logic pertaining to GrackleCtxPack -/// -//===----------------------------------------------------------------------===// - -#include "GrackleCtxPack.hpp" -#include "status_reporting.h" - -#include - -namespace grtest::param_detail { - -static bool set_str_(chemistry_data& my_chem, const std::string& name, - const char* val, std::size_t sz_with_nul, - StrAllocTracker* str_allocs) { - // NOTE: we should NOT directly modify characters held by field_ptr - const char** dest = const_cast( - local_chemistry_data_access_string(&my_chem, name.c_str())); - if (dest == nullptr) { - return false; // field is either not known or not a string - } - - // the following acts as a compiler hint (to suppress a warning) - GR_INTERNAL_REQUIRE((val == nullptr) != (sz_with_nul == 0), "compiler-hint"); - - if (str_allocs != nullptr) { - // (if applicable) allocate a new buffer and deallocate the old buffer - char* new_alloc = str_allocs->alloc_buf_and_free_old(sz_with_nul, *dest); - if (sz_with_nul > 0) { - std::memcpy(new_alloc, val, sz_with_nul - 1); - new_alloc[sz_with_nul - 1] = '\0'; - } - (*dest) = new_alloc; - } else { - (*dest) = val; - } - return true; -} - -bool set_str(chemistry_data& my_chem, const std::string& name, const char* val, - StrAllocTracker* str_allocs) { - std::size_t sz_with_nul = (val == nullptr) ? 0 : std::strlen(val) + 1; - return set_str_(my_chem, name, val, sz_with_nul, str_allocs); -} - -bool set_str(chemistry_data& my_chem, const std::string& name, - const std::string val, StrAllocTracker* str_allocs) { - return set_str_(my_chem, name, val.data(), val.size() + 1, str_allocs); -} - -} // namespace grtest::param_detail \ No newline at end of file diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index db8b58f39..2fca9d7de 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -61,43 +61,6 @@ class StrAllocTracker { } }; -/// Tries to set a string parameter tracked by @p my_param -/// -/// @param[in,out] my_chem Tracks various Grackle parameters -/// @param[in] name The name of the parameter getting updated -/// @param[in] val The value of the parameter -/// @param[in] str_allocs Tracks allocations of the strings held by @p my_chem -/// @returns true if successful and `false` if there was an error (e.g. @p name -/// isn't a known parameter or is a parameter that doesn't expect a string) -/// -/// @note -/// Oridinarily, this function will update @p str_allocs to hold a copy of -/// @p val and @p my_chem will be updated to store a pointer to that copy. When -/// @p str_allocs is a nullptr, @p my_chem is directly updated to track the data -/// referenced by @p val. -bool set_str(chemistry_data& my_chem, const std::string& name, const char* val, - StrAllocTracker* str_allocs); -bool set_str(chemistry_data& my_chem, const std::string& name, - const std::string& val, StrAllocTracker* str_allocs); - -inline bool set_int(chemistry_data& my_chem, const std::string& name, int val) { - int* dest = local_chemistry_data_access_int(&my_chem, name.c_str()); - if (dest == nullptr) { - return false; - } - (*dest) = val; - return true; -} - -inline bool set_double(chemistry_data& my_chem, const std::string& name, - double val) { - double* dest = local_chemistry_data_access_double(&my_chem, name.c_str()); - if (dest == nullptr) { - return false; - } - (*dest) = val; - return true; -} } // namespace param_detail diff --git a/tests/grtestutils/param.cpp b/tests/grtestutils/param.cpp new file mode 100644 index 000000000..054262152 --- /dev/null +++ b/tests/grtestutils/param.cpp @@ -0,0 +1,179 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Logic pertaining to ParamVal +/// +//===----------------------------------------------------------------------===// +#include "status_reporting.h" + +#include "param.hpp" +#include "GrackleCtxPack.hpp" // param_detail::StrAllocTracker +#include // std::strlen +#include // std::ostream +#include // std::false_type, std::is_same_v, std::decay_t + +namespace { // stuff inside an anonymous namespace is local to this file +// this is used to report a compile-time error in the lambda function that +// we use in std::visit (this is standard practice) +template +constexpr std::false_type always_false_{}; +} // anonymous namespace + +namespace grtest { + +std::string ParamVal::to_string() const { + std::string tmp; + // this lambda function is passed a reference to the value within this->val_ + auto get_string_repr_ = [&tmp](const auto& v) -> void { + using T = std::decay_t; + if constexpr (std::is_same_v || std::is_same_v) { + tmp = std::to_string(v); + } else if constexpr (std::is_same_v) { + tmp.reserve(v.size() + 2); + tmp += '"'; + tmp += v; + tmp += '"'; + } else if constexpr (std::is_same_v) { + tmp = "nullptr"; + } else { + static_assert(always_false_, "encountered unhandled type"); + } + }; + std::visit(get_string_repr_, this->val_); + + std::string out; + out.reserve(tmp.size() + 10); + out = "ParamVal("; + out += tmp; + out += ")"; + return out; +} + +void PrintTo(const ParamVal& p, std::ostream* os) { *os << p.to_string(); } + +bool ParamVal::is_equal(int val) const { + return std::holds_alternative(val_) && val == std::get(val_); +} + +bool ParamVal::is_equal(double val) const { + return std::holds_alternative(val_) && val == std::get(val_); +} + +bool ParamVal::is_equal(std::string_view val) const { + return std::holds_alternative(val_) && + val == std::get(val_); +} + +bool ParamVal::is_equal(const char* val) const { + if (val == nullptr) { + return std::holds_alternative(val_); + } + return is_equal(std::string_view(val)); +} + +namespace { // stuff inside an anonymous namespace is local to this file + +bool set_str_(chemistry_data& my_chem, const std::string& name, const char* val, + std::size_t sz_with_nul, + param_detail::StrAllocTracker* str_allocs) { + // NOTE: we should NOT directly modify characters held by field_ptr + const char** dest = const_cast( + local_chemistry_data_access_string(&my_chem, name.c_str())); + if (dest == nullptr) { + return false; // field is either not known or not a string + } + + // the following acts as a compiler hint (to suppress a warning) + GR_INTERNAL_REQUIRE((val == nullptr) != (sz_with_nul > 0), "compiler-hint"); + + if (str_allocs != nullptr) { + // (if applicable) allocate a new buffer and deallocate the old buffer + char* new_alloc = str_allocs->alloc_buf_and_free_old(sz_with_nul, *dest); + if (sz_with_nul > 0) { + std::memcpy(new_alloc, val, sz_with_nul - 1); + new_alloc[sz_with_nul - 1] = '\0'; + } + (*dest) = new_alloc; + } else { + (*dest) = val; + } + return true; +} + +/// Tries to set a string parameter tracked by @p val +/// +/// @param[in,out] my_chem Tracks various Grackle parameters +/// @param[in] name The name of the parameter getting updated +/// @param[in] val The value of the parameter +/// @param[in] str_allocs Tracks allocations of the strings held by @p my_chem +/// @returns true if successful and `false` if there was an error (e.g. @p name +/// isn't a known parameter or is a parameter that doesn't expect a string) +/// +/// @note +/// Ordinarily, this function will update @p str_allocs to hold a copy of +/// @p val and @p my_chem will be updated to store a pointer to that copy. When +/// @p str_allocs is a nullptr, @p my_chem is directly updated to track the data +/// referenced by @p val. +bool set_str(chemistry_data& my_chem, const std::string& name, const char* val, + param_detail::StrAllocTracker* str_allocs) { + std::size_t sz_with_nul = (val == nullptr) ? 0 : std::strlen(val) + 1; + return set_str_(my_chem, name, val, sz_with_nul, str_allocs); +} + +bool set_str(chemistry_data& my_chem, const std::string& name, + const std::string& val, + param_detail::StrAllocTracker* str_allocs) { + return set_str_(my_chem, name, val.data(), val.size() + 1, str_allocs); +} + +bool set_int(chemistry_data& my_chem, const std::string& name, int val) { + int* dest = local_chemistry_data_access_int(&my_chem, name.c_str()); + if (dest == nullptr) { + return false; + } + (*dest) = val; + return true; +} + +bool set_double(chemistry_data& my_chem, const std::string& name, double val) { + double* dest = local_chemistry_data_access_double(&my_chem, name.c_str()); + if (dest == nullptr) { + return false; + } + (*dest) = val; + return true; +} + +} // anonymous namespace + +bool set_param(chemistry_data& my_chem, const std::string& name, + const ParamVal& par_val, + param_detail::StrAllocTracker* str_allocs) { + bool success = false; + + // this lambda function is passed a reference to the value within par_val.val_ + auto set_val = [&success, &my_chem, &name, + str_allocs](const auto& v) -> void { + using T = std::decay_t; + if constexpr (std::is_same_v) { + success = set_int(my_chem, name, v); + } else if constexpr (std::is_same_v) { + success = set_double(my_chem, name, v); + } else if constexpr (std::is_same_v || + std::is_same_v) { + success = set_str(my_chem, name, v, str_allocs); + } else { + static_assert(always_false_, "encountered unhandled type"); + } + }; + + std::visit(set_val, par_val.val_); + return success; +} + +} // namespace grtest \ No newline at end of file diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/param.hpp new file mode 100644 index 000000000..7ea9465be --- /dev/null +++ b/tests/grtestutils/param.hpp @@ -0,0 +1,171 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_PARAM_HPP +#define GRTESTUTILS_PARAM_HPP + +#include "grackle.h" +#include +#include +#include +#include +#include + +namespace grtest { + +namespace param_detail { +// a forward declaration +class StrAllocTracker; +} // namespace param_detail + +/// Used to hold a chemistry_data parameter value (of arbitrary type) +/// +/// This is primarily intended to help construct a chemistry_data instance +class ParamVal { + friend bool set_param(chemistry_data& my_chem, const std::string& name, + const ParamVal& par_val, + param_detail::StrAllocTracker* str_allocs); + + using val_type = std::variant; + + // private attribute + val_type val_; + + static val_type coerce_c_string_(const char* s) { + return (s == nullptr) ? val_type(std::in_place_type) + : val_type(std::in_place_type, s); + } + +public: + // it isn't possible to have an empty parameter value + ParamVal() = delete; + + explicit ParamVal(int val) : val_(val) {} + explicit ParamVal(double val) : val_(val) {} + explicit ParamVal(std::string val) : val_(val) {} + explicit ParamVal(std::string_view val) : val_(std::string(val)) {} + explicit ParamVal(const char* val) : val_(ParamVal::coerce_c_string_(val)) {} + + /// return a string-representation of `this` + std::string to_string() const; + + /// teach googletest how to print the value + friend void PrintTo(const ParamVal& p, std::ostream* os); + + bool operator==(const ParamVal& other) const { return val_ == other.val_; } + bool operator!=(const ParamVal& other) const { return val_ != other.val_; } + + // all the following methods are primarily intended for debugging purposes + // (they are not essential to the core functionality) + + /// Try to access a pointer the contained value (intended for debugging) + /// + /// For example, `param_val.try_get()`, tries to access the value as an + /// integer + template + std::optional try_get() const { + T* ptr = std::get_if(&val_); + return (ptr == nullptr) ? std::nullopt : std::optional{*ptr}; + } + + /// Returns whether `this` holds an empty string + bool is_empty_string() const { + return std::holds_alternative(val_); + } + + /*!{*/ + /// Checks equivalence with the specified value + bool is_equal(int val) const; + bool is_equal(double val) const; + bool is_equal(std::string_view val) const; + bool is_equal(const char* val) const; + /*!}*/ +}; + +/// Use this whenever you want to make a sequence of key-value pairs +using ParamPair = std::pair; + +/// Nicely format a @ref ParamPair as a string +inline std::string to_string(const ParamPair& pair) { + std::string str_val = pair.second.to_string(); + std::string out; + out.reserve(pair.first.size() + str_val.size() + 6); + out += "{\""; + out += pair.first; + out += "\", "; + out += str_val; + out += '}'; + return out; +} + +/// Tries to update @p my_chem to hold the value specified by @p par_val +/// +/// @param[in,out] my_chem Tracks various Grackle parameters +/// @param[in] name The name of the parameter getting updated +/// @param[in] par_val The value of the parameter +/// @param[in] str_allocs Tracks allocations of the strings held by @p my_chem +/// @returns true if successful and `false` if there was an error (e.g. @p name +/// isn't a known parameter or is a parameter that doesn't expect a string) +/// +/// @note +/// When @p par_val holds a non-null string and @p name is a known parameter +/// holding a string: +/// - this function ordinarily updates @p str_allocs to hold a copy of +/// @p par_val and @p my_chem will be updated to store a pointer to that copy. +/// - When @p str_allocs is a nullptr, @p my_chem is directly updated to track +/// the data contained within @p par_val. +bool set_param(chemistry_data& my_chem, const std::string& name, + const ParamVal& par_val, + param_detail::StrAllocTracker* str_allocs); + +/// Tries to update @p my_chem to hold the value specified by @p par_val +/// +/// This is designed work with various standard library container types holding +/// key-value parameter name pairs. For concreteness, suppose we have a variable +/// called `my_params` with one of the following types: +/// - `std::vector>` (this is equivalent to +/// `std::vector`) +/// - `std::map` +/// - `std::unordered_map` +/// +/// For each of the cases, you would invoke +/// @code{C++} +/// set_params(my_params.cbegin(), my_params.cend(), my_chem, str_allocs); +/// @endcode +/// +/// @param[in] first, last the pair of iterators defining the range of parameter +/// name-value pairs that will be used to update my_chem. +/// @param[in,out] my_chem Tracks various Grackle parameters +/// @param[in] str_allocs Tracks allocations of the strings held by @p my_chem +/// @returns An empty optional if successful. Otherwise, it returns the name +/// of the parameter that couldn't be updated. +/// +/// @tparam It The type of the iterator. When str_allocs is a `nullptr` this +/// must be a ForwardIterator (i.e. after incrementing the iterator, +/// previous values must remain valid). Otherwise, this can also be an +/// InputIterator +template +std::optional set_params( + It first, It last, chemistry_data& my_chem, + param_detail::StrAllocTracker* str_allocs) { + for (It it = first; it != last; ++it) { + const std::string& name = it->first; + const ParamVal& val = it->second; + if (!set_param(my_chem, name, val, str_allocs)) { + return std::optional(name); + } + } + return std::nullopt; // we succeeded +} + +} // namespace grtest + +#endif // GRTESTUTILS_PARAM_HPP From 1ea8dd8cfb132c869f1c0fe076b55d5288047ad8 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 24 Jan 2026 14:23:40 -0500 Subject: [PATCH 04/20] grtestutils: refactor presets to use ParamVal --- tests/grtestutils/GrackleCtxPack.hpp | 46 +++++++++++++------ tests/grtestutils/preset.cpp | 69 +++++++++++++++------------- tests/grtestutils/preset.hpp | 10 ++-- tests/grtestutils/utils.cpp | 22 +++++---- tests/grtestutils/utils.hpp | 20 +++----- 5 files changed, 96 insertions(+), 71 deletions(-) diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index 2fca9d7de..601f2a12f 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -61,7 +61,6 @@ class StrAllocTracker { } }; - } // namespace param_detail /// Tracks the group of Grackle objects needed for executing API functions @@ -79,6 +78,8 @@ class StrAllocTracker { class GrackleCtxPack { /// units used for initializing chemistry_data code_units initial_units_; + /// tracks string allocations for @ref my_chemistry_ + param_detail::StrAllocTracker str_allocs_; /// the fully initialized chemistry_data instance std::unique_ptr my_chemistry_; /// the fully initialized chemistry_data_storage_instance @@ -115,30 +116,49 @@ class GrackleCtxPack { /// create an initialized instance from a preset static GrackleCtxPack create(const FullConfPreset& preset, InitStatus* status) { - std::unique_ptr my_chemistry(new chemistry_data); - InitStatus tmp = - setup_chemistry_data_from_preset(my_chemistry.get(), preset.chemistry); - if (tmp != InitStatus::success) { + auto erroneous_exit = [&status](InitStatus new_status) { if (status != nullptr) { - *status = tmp; + *status = new_status; } - return GrackleCtxPack(); // return an unitialized instance + return GrackleCtxPack(); + }; + + // allocate chemistry_data and set the defaults + std::unique_ptr my_chem(new chemistry_data); + if (local_initialize_chemistry_parameters(my_chem.get()) != GR_SUCCESS) { + return erroneous_exit(InitStatus::generic_fail); } + // lookup the parameters associated with the preset + std::pair, InitStatus> chem_preset_rslt = + get_chem_preset_vals_(preset.chemistry); + if (chem_preset_rslt.second != InitStatus::success) { + return erroneous_exit(chem_preset_rslt.second); + } + const std::vector& params = chem_preset_rslt.first; + + // update my_chem with values from the preset + param_detail::StrAllocTracker str_allocs; + std::optional bad_param = + set_params(params.begin(), params.end(), *my_chem, &str_allocs); + if (bad_param.has_value()) { + // todo: we probably want to tell the caller about the bad parameter + return erroneous_exit(InitStatus::generic_fail); + } + + // set up chemistry_data_storage code_units initial_unit = setup_initial_unit(preset.unit); std::unique_ptr my_rates( new chemistry_data_storage); - if (local_initialize_chemistry_data(my_chemistry.get(), my_rates.get(), + if (local_initialize_chemistry_data(my_chem.get(), my_rates.get(), &initial_unit) != GR_SUCCESS) { - if (status != nullptr) { - *status = InitStatus::generic_fail; - } - return GrackleCtxPack(); // return an unitialized instance + return erroneous_exit(InitStatus::generic_fail); } GrackleCtxPack out; out.initial_units_ = initial_unit; - out.my_chemistry_ = std::move(my_chemistry); + out.str_allocs_ = std::move(str_allocs); + out.my_chemistry_ = std::move(my_chem); out.my_rates_ = std::move(my_rates); return out; } diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index fedf0f96d..4ccfc8a0f 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -10,12 +10,15 @@ /// //===----------------------------------------------------------------------===// +#include "./param.hpp" #include "./preset.hpp" #include "./utils.hpp" #include "grackle.h" #include "status_reporting.h" // GR_INTERNAL_UNREACHABLE_ERROR +#include + namespace grtest { static std::string to_string(const grtest::ChemPreset& preset) { switch (preset) { @@ -34,50 +37,52 @@ static std::string to_string(const grtest::ChemPreset& preset) { GR_INTERNAL_UNREACHABLE_ERROR(); } -InitStatus setup_chemistry_data_from_preset(chemistry_data* my_chem, - ChemPreset preset) { - if (local_initialize_chemistry_parameters(my_chem) != GR_SUCCESS) { - return InitStatus::generic_fail; - } +std::pair, InitStatus> get_chem_preset_vals_( + ChemPreset preset) { + std::vector> v = { + {"use_grackle", ParamVal(1)}, // chemistry on + {"use_isrf_field", ParamVal(1)}, + {"with_radiative_cooling", ParamVal(1)}, // cooling on + {"metal_cooling", ParamVal(1)}, // metal cooling on + {"UVbackground", ParamVal(1)}, // UV background on + }; - if (!set_standard_datafile(*my_chem, "CloudyData_UVB=HM2012.h5")) { - return InitStatus::standard_datafile_notfound; + std::optional maybe_datafile = + get_standard_datafile("CloudyData_UVB=HM2012.h5"); + if (!maybe_datafile.has_value()) { + return {{}, InitStatus::standard_datafile_notfound}; + } else { + v.emplace_back("grackle_data_file", maybe_datafile.value()); } - my_chem->use_grackle = 1; // chemistry on - my_chem->use_isrf_field = 1; - my_chem->with_radiative_cooling = 1; // cooling on - my_chem->metal_cooling = 1; // metal cooling on - my_chem->UVbackground = 1; // UV background on - switch (preset) { case ChemPreset::primchem0: { - my_chem->primordial_chemistry = 0; - my_chem->dust_chemistry = 0; - return InitStatus::success; + v.emplace_back("primordial_chemistry", 0); + v.emplace_back("dust_chemistry", 0); + return {v, InitStatus::success}; } case ChemPreset::primchem1: { - my_chem->primordial_chemistry = 1; - my_chem->dust_chemistry = 1; - return InitStatus::success; + v.emplace_back("primordial_chemistry", 1); + v.emplace_back("dust_chemistry", 1); + return {v, InitStatus::success}; } case ChemPreset::primchem2: { - my_chem->primordial_chemistry = 2; - my_chem->dust_chemistry = 1; - return InitStatus::success; + v.emplace_back("primordial_chemistry", 2); + v.emplace_back("dust_chemistry", 1); + return {v, InitStatus::success}; } case ChemPreset::primchem3: { - my_chem->primordial_chemistry = 3; - my_chem->dust_chemistry = 1; - return InitStatus::success; + v.emplace_back("primordial_chemistry", 3); + v.emplace_back("dust_chemistry", 1); + return {v, InitStatus::success}; } case ChemPreset::primchem4_dustspecies3: { - my_chem->primordial_chemistry = 4; - my_chem->dust_chemistry = 1; - my_chem->metal_chemistry = 1; - my_chem->dust_species = 3; - my_chem->use_dust_density_field = 1; - return InitStatus::success; + v.emplace_back("primordial_chemistry", 4); + v.emplace_back("dust_chemistry", 1); + v.emplace_back("metal_chemistry", 1); + v.emplace_back("dust_species", 3); + v.emplace_back("use_dust_density_field", 1); + return {v, InitStatus::success}; } } GR_INTERNAL_UNREACHABLE_ERROR(); @@ -117,4 +122,4 @@ void PrintTo(const grtest::FullConfPreset& preset, std::ostream* os) { << to_string(preset.unit) << '}'; } -} // namespace grtest \ No newline at end of file +} // namespace grtest diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index b1278d175..8e5b96478 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -13,9 +13,9 @@ #define GRTESTUTILS_PRESET_HPP #include "grackle.h" -#include -#include -#include +#include "./param.hpp" +#include +#include namespace grtest { @@ -39,8 +39,8 @@ enum class ChemPreset { }; /// override the settings of my_chem based on the specified preset -InitStatus setup_chemistry_data_from_preset(chemistry_data* my_chem, - ChemPreset preset); +std::pair, InitStatus> get_chem_preset_vals_( + ChemPreset preset); /// Preset for constructing the code_unit instance used for initializing the /// Grackle Solver diff --git a/tests/grtestutils/utils.cpp b/tests/grtestutils/utils.cpp index d4781249f..3665911f4 100644 --- a/tests/grtestutils/utils.cpp +++ b/tests/grtestutils/utils.cpp @@ -22,12 +22,11 @@ static const char* const standard_data_files[N_STANDARD_DATAFILES] = { stringify(GR_DATADIR) "/cloudy_metals_2008_3D.h5" }; -bool grtest::set_standard_datafile( - chemistry_data& my_chemistry, const char* datafile -) { +/// returns the string-literal for the standard data file +static const char* standard_datafile_literal_(const char* datafile) { - if (datafile==NULL) { - return false; // we should probably abort the program with an error + if (datafile==nullptr) { + return nullptr; } // we get the number of characters in the prefix-path @@ -38,9 +37,16 @@ bool grtest::set_standard_datafile( for (int i = 0; i < N_STANDARD_DATAFILES; i++){ if (std::strcmp(datafile, standard_data_files[i]+prefix_len) == 0) { - my_chemistry.grackle_data_file = standard_data_files[i]; - return true; + return standard_data_files[i]; } } - return false; + return nullptr; } + +std::optional grtest::get_standard_datafile(const char* datafile) { + const char* tmp = standard_datafile_literal_(datafile); + if (tmp==nullptr) { + return std::nullopt; + } + return std::optional(tmp); +} \ No newline at end of file diff --git a/tests/grtestutils/utils.hpp b/tests/grtestutils/utils.hpp index 72e4e77fa..b0fc12bcc 100644 --- a/tests/grtestutils/utils.hpp +++ b/tests/grtestutils/utils.hpp @@ -3,25 +3,19 @@ #ifndef GRTEST_UTILS_HPP #define GRTEST_UTILS_HPP -#include #include +#include +#include +#include + namespace grtest { -/// this function records the desired standard datafile within the -/// chemistry_data struct. It deals with the minutia of making sure that +/// this function returns the desired standard datafile. It deals with the minutia of making sure that /// grackle can find the standardized data-file /// -/// @returns true if successful or false if unsuccessful -/// -/// @note -/// For the sake of forward compatability (we will probably change the -/// implementation if we merge PRs 235, 237, and 246) you should: -/// - only pass string literals (or the addresses of string-literals) as the -/// this function's datafile arg -/// - AND never deallocate my_chemistry.datafile after calling this function -/// (there won't be a memory leak) -bool set_standard_datafile(chemistry_data& my_chemistry, const char* datafile); +/// @returns An empty optional if unsuccessful +std::optional get_standard_datafile(const char* datafile); } From 61c7fcf2d42e913e788faf37165cb3055b2c4724 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 23 Jan 2026 08:50:39 -0500 Subject: [PATCH 05/20] make it possible to more concisely initialize a vector of key-value pairs --- tests/grtestutils/param.hpp | 49 ++++++++++++++++++++++++++++++++++++ tests/grtestutils/preset.cpp | 14 +++++------ 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/param.hpp index 7ea9465be..e3c5ee7be 100644 --- a/tests/grtestutils/param.hpp +++ b/tests/grtestutils/param.hpp @@ -13,11 +13,13 @@ #define GRTESTUTILS_PARAM_HPP #include "grackle.h" +#include #include #include #include #include #include +#include namespace grtest { @@ -106,6 +108,53 @@ inline std::string to_string(const ParamPair& pair) { return out; } +namespace param_detail { + +/// this **ONLY** exists to help implement @ref make_ParamPair_vec +struct ParamPairInit { + ParamPair pair; + + template + ParamPairInit(const char* k, V&& v) : pair(k, ParamVal(std::forward(v))) {} +}; + +} // namespace param_detail + +/// A convenience function for easier construction of a vector of Param pairs +/// +/// This function lets you write code like +/// @code{C++} +/// std::vector v = make_ParamPair_vec({ +/// {"use_grackle", 1}, +/// {"with_radiative_cooling", 1}, +/// {"metal_cooling", 1}, +/// {"UVbackground", 1}, +/// {"primordial_chemistry", 2} +/// }); +/// @endcode +/// +/// We could definitely get rid of this function, but then we would need to +/// rewrite the above snippet as: +/// @code{C++} +/// std::vector v = { +/// {"use_grackle", ParamVal(1)}, +/// {"with_radiative_cooling", ParamVal(1)}, +/// {"metal_cooling", ParamVal(1)}, +/// {"UVbackground", ParamVal(1)}, +/// {"primordial_chemistry", ParamVal(2)} +/// }; +inline std::vector make_ParamPair_vec( + std::initializer_list l) { + // if we were a little more clever, we might be able to get rid of an + // intermediate allocation when constructing ParamPairInit... + std::vector v; + v.reserve(l.size()); + for (const param_detail::ParamPairInit& p : l) { + v.push_back(p.pair); + } + return v; +} + /// Tries to update @p my_chem to hold the value specified by @p par_val /// /// @param[in,out] my_chem Tracks various Grackle parameters diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index 4ccfc8a0f..bd19e0761 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -39,13 +39,13 @@ static std::string to_string(const grtest::ChemPreset& preset) { std::pair, InitStatus> get_chem_preset_vals_( ChemPreset preset) { - std::vector> v = { - {"use_grackle", ParamVal(1)}, // chemistry on - {"use_isrf_field", ParamVal(1)}, - {"with_radiative_cooling", ParamVal(1)}, // cooling on - {"metal_cooling", ParamVal(1)}, // metal cooling on - {"UVbackground", ParamVal(1)}, // UV background on - }; + std::vector> v = make_ParamPair_vec({ + {"use_grackle", 1}, // chemistry on + {"use_isrf_field", 1}, + {"with_radiative_cooling", 1}, // cooling on + {"metal_cooling", 1}, // metal cooling on + {"UVbackground", 1}, // UV background on + }); std::optional maybe_datafile = get_standard_datafile("CloudyData_UVB=HM2012.h5"); From af25b64fc519546375093d3dab3474b4fad5e53d Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 23 Jan 2026 09:45:18 -0500 Subject: [PATCH 06/20] rename FullConfPreset->SimpleConfPreset --- tests/grtestutils/GrackleCtxPack.hpp | 2 +- tests/grtestutils/googletest/fixtures.hpp | 4 ++-- tests/grtestutils/preset.cpp | 2 +- tests/grtestutils/preset.hpp | 6 +++--- tests/unit/test_api_ratequery.cpp | 14 +++++++------- tests/unit/test_ghost_zone.cpp | 10 +++++----- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index 601f2a12f..0d26d6487 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -114,7 +114,7 @@ class GrackleCtxPack { chemistry_data_storage* my_rates() { return this->my_rates_.get(); } /// create an initialized instance from a preset - static GrackleCtxPack create(const FullConfPreset& preset, + static GrackleCtxPack create(const SimpleConfPreset& preset, InitStatus* status) { auto erroneous_exit = [&status](InitStatus new_status) { if (status != nullptr) { diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index fc72c4a3a..6b8edacd7 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -44,7 +44,7 @@ class ConfigPresetFixture : public testing::Test { // called immediately after the constructor (but before the test-case) grtest::InitStatus status; - pack = GrackleCtxPack::create(FullConfPreset{chem_preset, unit_preset}, + pack = GrackleCtxPack::create(SimpleConfPreset{chem_preset, unit_preset}, &status); if (!pack.is_initialized()) { if (status == InitStatus::standard_datafile_notfound) { @@ -74,7 +74,7 @@ class ConfigPresetFixture : public testing::Test { /// this class, OR /// 2. make a subclass, named `MyFeatureTest`, of this class class ParametrizedConfigPresetFixture - : public testing::TestWithParam { + : public testing::TestWithParam { protected: void SetUp() override { // called immediately after the constructor (but before the test-case) diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index bd19e0761..2ff206dd1 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -117,7 +117,7 @@ code_units setup_initial_unit(InitialUnitPreset preset) { GR_INTERNAL_UNREACHABLE_ERROR(); } -void PrintTo(const grtest::FullConfPreset& preset, std::ostream* os) { +void PrintTo(const grtest::SimpleConfPreset& preset, std::ostream* os) { *os << "Preset{" << to_string(preset.chemistry) << ',' << to_string(preset.unit) << '}'; } diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index 8e5b96478..a9156252a 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -55,13 +55,13 @@ enum class InitialUnitPreset { code_units setup_initial_unit(InitialUnitPreset preset); /// Represents the preset for creating a GrackleCtxPack -struct FullConfPreset { +struct SimpleConfPreset { ChemPreset chemistry; InitialUnitPreset unit; }; -// teach googletest how to print FullConfPreset -void PrintTo(const FullConfPreset& preset, std::ostream* os); +// teach googletest how to print SimpleConfPreset +void PrintTo(const SimpleConfPreset& preset, std::ostream* os); } // namespace grtest diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index 02d0882bd..f163f192a 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -78,15 +78,15 @@ TEST_P(ParametrizedRateQueryTest, ConsistentIDs) { } using grtest::ChemPreset; -using grtest::FullConfPreset; using grtest::InitialUnitPreset; +using grtest::SimpleConfPreset; INSTANTIATE_TEST_SUITE_P( /* 1st arg is intentionally empty */, ParametrizedRateQueryTest, ::testing::Values( - FullConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, - FullConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, - FullConfPreset{ChemPreset::primchem2, InitialUnitPreset::simple_z0}, - FullConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}, - FullConfPreset{ChemPreset::primchem4_dustspecies3, - InitialUnitPreset::simple_z0})); + SimpleConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem2, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem4_dustspecies3, + InitialUnitPreset::simple_z0})); diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index 16628721f..317b055b7 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -342,15 +342,15 @@ TEST_P(APIGhostZoneTest, GridZoneStartEnd) { } -using grtest::FullConfPreset; +using grtest::SimpleConfPreset; using grtest::ChemPreset; using grtest::InitialUnitPreset; INSTANTIATE_TEST_SUITE_P( /* 1st arg is intentionally empty */, APIGhostZoneTest, ::testing::Values( - FullConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, - FullConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, - FullConfPreset{ChemPreset::primchem2, InitialUnitPreset::simple_z0}, - FullConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}) + SimpleConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem2, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}) ); From 151cc5f6f93f00fd5c65a30583087739685e22de Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 23 Jan 2026 16:29:16 -0500 Subject: [PATCH 07/20] grtestutils: introduce Status type --- tests/grtestutils/CMakeLists.txt | 1 + tests/grtestutils/GrackleCtxPack.hpp | 43 ++++++------ tests/grtestutils/googletest/fixtures.hpp | 39 +++++------ tests/grtestutils/param.hpp | 10 +-- tests/grtestutils/preset.cpp | 14 ++-- tests/grtestutils/preset.hpp | 10 +-- tests/grtestutils/status.cpp | 68 +++++++++++++++++++ tests/grtestutils/status.hpp | 83 +++++++++++++++++++++++ 8 files changed, 203 insertions(+), 65 deletions(-) create mode 100644 tests/grtestutils/status.cpp create mode 100644 tests/grtestutils/status.hpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index bf12b77f4..cda5b1e78 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -85,6 +85,7 @@ add_library(grtestutils os.hpp os.cpp param.hpp param.cpp preset.hpp preset.cpp + status.hpp status.cpp utils.hpp utils.cpp # files in the googletest subdirectory (reminder: should just be headers!) diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index 0d26d6487..172be18b0 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -12,7 +12,8 @@ #ifndef GRTESTUTILS_GRACKLECTXPACK_HPP #define GRTESTUTILS_GRACKLECTXPACK_HPP -#include "preset.hpp" +#include "./preset.hpp" +#include "./status.hpp" #include // std::remove_if #include // std::unique_ptr @@ -114,36 +115,29 @@ class GrackleCtxPack { chemistry_data_storage* my_rates() { return this->my_rates_.get(); } /// create an initialized instance from a preset - static GrackleCtxPack create(const SimpleConfPreset& preset, - InitStatus* status) { - auto erroneous_exit = [&status](InitStatus new_status) { - if (status != nullptr) { - *status = new_status; - } - return GrackleCtxPack(); - }; - + static std::pair create( + const SimpleConfPreset& preset) { // allocate chemistry_data and set the defaults std::unique_ptr my_chem(new chemistry_data); if (local_initialize_chemistry_parameters(my_chem.get()) != GR_SUCCESS) { - return erroneous_exit(InitStatus::generic_fail); + return {GrackleCtxPack(), + error::Adhoc("initialize_chemistry_parameters failed")}; } // lookup the parameters associated with the preset - std::pair, InitStatus> chem_preset_rslt = + std::pair, Status> chem_preset_rslt = get_chem_preset_vals_(preset.chemistry); - if (chem_preset_rslt.second != InitStatus::success) { - return erroneous_exit(chem_preset_rslt.second); + if (!chem_preset_rslt.second.is_ok()) { + return {GrackleCtxPack(), chem_preset_rslt.second}; } const std::vector& params = chem_preset_rslt.first; // update my_chem with values from the preset param_detail::StrAllocTracker str_allocs; - std::optional bad_param = + Status status = set_params(params.begin(), params.end(), *my_chem, &str_allocs); - if (bad_param.has_value()) { - // todo: we probably want to tell the caller about the bad parameter - return erroneous_exit(InitStatus::generic_fail); + if (!status.is_ok()) { + return {GrackleCtxPack(), status}; } // set up chemistry_data_storage @@ -152,14 +146,15 @@ class GrackleCtxPack { new chemistry_data_storage); if (local_initialize_chemistry_data(my_chem.get(), my_rates.get(), &initial_unit) != GR_SUCCESS) { - return erroneous_exit(InitStatus::generic_fail); + return {GrackleCtxPack(), + error::Adhoc("initialize_chemistry_data failed")}; } - GrackleCtxPack out; - out.initial_units_ = initial_unit; - out.str_allocs_ = std::move(str_allocs); - out.my_chemistry_ = std::move(my_chem); - out.my_rates_ = std::move(my_rates); + std::pair out{GrackleCtxPack(), OkStatus()}; + out.first.initial_units_ = initial_unit; + out.first.str_allocs_ = std::move(str_allocs); + out.first.my_chemistry_ = std::move(my_chem); + out.first.my_rates_ = std::move(my_rates); return out; } }; diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index 6b8edacd7..00e5270f0 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -20,6 +20,9 @@ #include "../GrackleCtxPack.hpp" #include "../preset.hpp" +#include "../status.hpp" + +#include // std::move namespace grtest { @@ -43,17 +46,14 @@ class ConfigPresetFixture : public testing::Test { void SetUp() override { // called immediately after the constructor (but before the test-case) - grtest::InitStatus status; - pack = GrackleCtxPack::create(SimpleConfPreset{chem_preset, unit_preset}, - &status); - if (!pack.is_initialized()) { - if (status == InitStatus::standard_datafile_notfound) { - GTEST_SKIP() - << "something went before initialization while searching for a " - << "standard datafile"; - } else { - FAIL() << "Error in initialize_chemistry_data."; - } + std::pair tmp = + GrackleCtxPack::create(SimpleConfPreset{chem_preset, unit_preset}); + if (tmp.second.is_ok()) { + pack = std::move(tmp.first); + } else if (tmp.second.is_missing_std_file()) { + GTEST_SKIP() << tmp.second.to_string(); + } else { + FAIL() << tmp.second.to_string(); } } @@ -78,16 +78,13 @@ class ParametrizedConfigPresetFixture protected: void SetUp() override { // called immediately after the constructor (but before the test-case) - grtest::InitStatus status; - pack = GrackleCtxPack::create(GetParam(), &status); - if (!pack.is_initialized()) { - if (status == InitStatus::standard_datafile_notfound) { - GTEST_SKIP() - << "something went before initialization while searching for a " - << "standard datafile"; - } else { - FAIL() << "Error in initialize_chemistry_data."; - } + std::pair tmp = GrackleCtxPack::create(GetParam()); + if (tmp.second.is_ok()) { + pack = std::move(tmp.first); + } else if (tmp.second.is_missing_std_file()) { + GTEST_SKIP() << tmp.second.to_string(); + } else { + FAIL() << tmp.second.to_string(); } } diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/param.hpp index e3c5ee7be..50af101f1 100644 --- a/tests/grtestutils/param.hpp +++ b/tests/grtestutils/param.hpp @@ -13,6 +13,7 @@ #define GRTESTUTILS_PARAM_HPP #include "grackle.h" +#include "./status.hpp" #include #include #include @@ -202,17 +203,16 @@ bool set_param(chemistry_data& my_chem, const std::string& name, /// previous values must remain valid). Otherwise, this can also be an /// InputIterator template -std::optional set_params( - It first, It last, chemistry_data& my_chem, - param_detail::StrAllocTracker* str_allocs) { +Status set_params(It first, It last, chemistry_data& my_chem, + param_detail::StrAllocTracker* str_allocs) { for (It it = first; it != last; ++it) { const std::string& name = it->first; const ParamVal& val = it->second; if (!set_param(my_chem, name, val, str_allocs)) { - return std::optional(name); + return error::Param(name); } } - return std::nullopt; // we succeeded + return OkStatus(); } } // namespace grtest diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index 2ff206dd1..0152bc0b3 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -37,7 +37,7 @@ static std::string to_string(const grtest::ChemPreset& preset) { GR_INTERNAL_UNREACHABLE_ERROR(); } -std::pair, InitStatus> get_chem_preset_vals_( +std::pair, Status> get_chem_preset_vals_( ChemPreset preset) { std::vector> v = make_ParamPair_vec({ {"use_grackle", 1}, // chemistry on @@ -50,7 +50,7 @@ std::pair, InitStatus> get_chem_preset_vals_( std::optional maybe_datafile = get_standard_datafile("CloudyData_UVB=HM2012.h5"); if (!maybe_datafile.has_value()) { - return {{}, InitStatus::standard_datafile_notfound}; + return {{}, error::MissingStdFile()}; } else { v.emplace_back("grackle_data_file", maybe_datafile.value()); } @@ -59,22 +59,22 @@ std::pair, InitStatus> get_chem_preset_vals_( case ChemPreset::primchem0: { v.emplace_back("primordial_chemistry", 0); v.emplace_back("dust_chemistry", 0); - return {v, InitStatus::success}; + return {v, OkStatus()}; } case ChemPreset::primchem1: { v.emplace_back("primordial_chemistry", 1); v.emplace_back("dust_chemistry", 1); - return {v, InitStatus::success}; + return {v, OkStatus()}; } case ChemPreset::primchem2: { v.emplace_back("primordial_chemistry", 2); v.emplace_back("dust_chemistry", 1); - return {v, InitStatus::success}; + return {v, OkStatus()}; } case ChemPreset::primchem3: { v.emplace_back("primordial_chemistry", 3); v.emplace_back("dust_chemistry", 1); - return {v, InitStatus::success}; + return {v, OkStatus()}; } case ChemPreset::primchem4_dustspecies3: { v.emplace_back("primordial_chemistry", 4); @@ -82,7 +82,7 @@ std::pair, InitStatus> get_chem_preset_vals_( v.emplace_back("metal_chemistry", 1); v.emplace_back("dust_species", 3); v.emplace_back("use_dust_density_field", 1); - return {v, InitStatus::success}; + return {v, OkStatus()}; } } GR_INTERNAL_UNREACHABLE_ERROR(); diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index a9156252a..808f3c52e 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -14,18 +14,12 @@ #include "grackle.h" #include "./param.hpp" +#include "./status.hpp" #include #include namespace grtest { -/// this only exists so that we can determine the reason that a test fails -enum class InitStatus { - success, - generic_fail, - standard_datafile_notfound, -}; - /// represents different presets for initializing chemistry_data /// /// @note @@ -39,7 +33,7 @@ enum class ChemPreset { }; /// override the settings of my_chem based on the specified preset -std::pair, InitStatus> get_chem_preset_vals_( +std::pair, Status> get_chem_preset_vals_( ChemPreset preset); /// Preset for constructing the code_unit instance used for initializing the diff --git a/tests/grtestutils/status.cpp b/tests/grtestutils/status.cpp new file mode 100644 index 000000000..081461723 --- /dev/null +++ b/tests/grtestutils/status.cpp @@ -0,0 +1,68 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Define the Status class +/// +//===----------------------------------------------------------------------===// +#include "status.hpp" + +#include "status_reporting.h" // GR_INTERNAL_UNREACHABLE_ERROR +#include // std::exchange + +namespace grtest { + +namespace status_detail { + +/// This mirrors the internal structure of Status. +struct TmpStatus { + StatusKind kind; + std::string s; +}; + +} // namespace status_detail + +Status::Status(status_detail::TmpStatus&& tmp) + : kind(tmp.kind), s(std::exchange(tmp.s, std::string())) {} + +Status error::MissingStdFile() { + return Status(status_detail::TmpStatus{ + status_detail::StatusKind::MISSING_STD_FILE, ""}); +} + +Status error::Param(std::string param_name) { + return Status(status_detail::TmpStatus{status_detail::StatusKind::PARAM, + std::move(param_name)}); +} + +Status error::Adhoc(std::string message) { + return Status(status_detail::TmpStatus{status_detail::StatusKind::ADHOC, + std::move(message)}); +} + +std::string Status::to_string() const { + switch (kind) { + case status_detail::StatusKind::OK: + return "OkStatus"; + case status_detail::StatusKind::MISSING_STD_FILE: { + return ( + "something went wrong with the test harness routine for looking up a " + "standard datafile"); + } + case status_detail::StatusKind::PARAM: { + return ( + "the parameter, \"" + this->s + + "\" isn't known to Grackle, or was associated with the wrong type"); + } + case status_detail::StatusKind::ADHOC: + return s; + default: + GR_INTERNAL_UNREACHABLE_ERROR(); + } +} + +} // namespace grtest \ No newline at end of file diff --git a/tests/grtestutils/status.hpp b/tests/grtestutils/status.hpp new file mode 100644 index 000000000..0e4ddee7b --- /dev/null +++ b/tests/grtestutils/status.hpp @@ -0,0 +1,83 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Declare the Status class +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_STATUS_HPP +#define GRTESTUTILS_STATUS_HPP + +#include + +namespace grtest { + +namespace status_detail { + +struct TmpStatus; // forward declaration + +enum class StatusKind { OK, MISSING_STD_FILE, PARAM, ADHOC }; + +} // namespace status_detail + +/// An instances can be used to denote context when returning from functions +/// +/// @par Aside +/// Frankly, I would prefer if we used std::expected (or a simplified backport) +/// and made this only represent errors, but we can always refactor later +class Status { + /// tracks the kind of status + status_detail::StatusKind kind; + + /// for certain kinds of statuses, this provides extra context + std::string s; + +public: + Status() : kind(status_detail::StatusKind::OK), s() {} + + /// this constructor is used by factory functions + explicit Status(status_detail::TmpStatus&& tmp); + + /// checks if the status is ok + bool is_ok() const { return kind == status_detail::StatusKind::OK; } + + /// checks if the status is an error + bool is_err() const { return !is_ok(); } + + /// coerce to a string representation + std::string to_string() const; + + /// Return true when `this` indicates that a standard datafile can't be found + bool is_missing_std_file() const { + return kind == status_detail::StatusKind::MISSING_STD_FILE; + } +}; + +/// construct an Ok status +inline Status OkStatus() { return Status(); } + +/// This namespace holds factory functions for @ref Status instances that +/// denote errors +namespace error { + +/// Indicates that standard datafiles are missing +Status MissingStdFile(); + +/// There was an error setting the specified parameter name. +/// +/// This means that the parameter wasn't known or the associated value had the +/// wrong type +Status Param(std::string param_name); + +/// Specifies a generic string error message +Status Adhoc(std::string message); + +} // namespace error + +} // namespace grtest + +#endif // GRTESTUTILS_STATUS_HPP From 26139f24d6fd1628f7318cdb94aeeeb6cd55386f Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 23 Jan 2026 17:11:35 -0500 Subject: [PATCH 08/20] relocate some logic and address compiler warning --- tests/grtestutils/CMakeLists.txt | 8 ++-- tests/grtestutils/GrackleCtxPack.cpp | 58 ++++++++++++++++++++++++++++ tests/grtestutils/GrackleCtxPack.hpp | 47 ++-------------------- 3 files changed, 66 insertions(+), 47 deletions(-) create mode 100644 tests/grtestutils/GrackleCtxPack.cpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index cda5b1e78..3cd85dc2b 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -80,12 +80,12 @@ add_library(grtestutils # files outside of the googletest subdirectory # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp - GrackleCtxPack.hpp + GrackleCtxPack.cpp GrackleCtxPack.hpp iterator_adaptor.hpp os.hpp os.cpp - param.hpp param.cpp - preset.hpp preset.cpp - status.hpp status.cpp + param.cpp param.hpp + preset.cpp preset.hpp + status.cpp status.hpp utils.hpp utils.cpp # files in the googletest subdirectory (reminder: should just be headers!) diff --git a/tests/grtestutils/GrackleCtxPack.cpp b/tests/grtestutils/GrackleCtxPack.cpp new file mode 100644 index 000000000..308fb91e5 --- /dev/null +++ b/tests/grtestutils/GrackleCtxPack.cpp @@ -0,0 +1,58 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Help implement GrackleCtxPack +/// +//===----------------------------------------------------------------------===// + +#include "GrackleCtxPack.hpp" + +namespace grtest { + +std::pair GrackleCtxPack::create( + const SimpleConfPreset& preset) { + // allocate chemistry_data and set the defaults + std::unique_ptr my_chem(new chemistry_data); + if (local_initialize_chemistry_parameters(my_chem.get()) != GR_SUCCESS) { + return {GrackleCtxPack(), + error::Adhoc("initialize_chemistry_parameters failed")}; + } + + // lookup the parameters associated with the preset + std::pair, Status> chem_preset_rslt = + get_chem_preset_vals_(preset.chemistry); + if (!chem_preset_rslt.second.is_ok()) { + return {GrackleCtxPack(), chem_preset_rslt.second}; + } + const std::vector& params = chem_preset_rslt.first; + + // update my_chem with values from the preset + param_detail::StrAllocTracker str_allocs; + Status status = + set_params(params.begin(), params.end(), *my_chem, &str_allocs); + if (!status.is_ok()) { + return {GrackleCtxPack(), status}; + } + + // set up chemistry_data_storage + code_units initial_unit = setup_initial_unit(preset.unit); + std::unique_ptr my_rates(new chemistry_data_storage); + if (local_initialize_chemistry_data(my_chem.get(), my_rates.get(), + &initial_unit) != GR_SUCCESS) { + return {GrackleCtxPack(), error::Adhoc("initialize_chemistry_data failed")}; + } + + std::pair out{GrackleCtxPack(), OkStatus()}; + out.first.initial_units_ = initial_unit; + out.first.str_allocs_ = std::move(str_allocs); + out.first.my_chemistry_ = std::move(my_chem); + out.first.my_rates_ = std::move(my_rates); + return out; +} + +} // namespace grtest diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index 172be18b0..f086ad636 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -88,13 +88,14 @@ class GrackleCtxPack { public: /// Construct an uninitialized instance - GrackleCtxPack() : my_chemistry_(nullptr), my_rates_(nullptr) {} + GrackleCtxPack() + : initial_units_{}, my_chemistry_(nullptr), my_rates_(nullptr) {} GrackleCtxPack(GrackleCtxPack&&) = default; GrackleCtxPack& operator=(GrackleCtxPack&&) = default; // forbid copy and assignment operations... - // -> we could re-enable them if we wanted to be able add to clone the + // -> we could re-enable them if we wanted to be able to add to clone the // types (or if we wanted to internally use std::shared_ptr GrackleCtxPack(const GrackleCtxPack&) = delete; GrackleCtxPack& operator=(const GrackleCtxPack&) = delete; @@ -116,47 +117,7 @@ class GrackleCtxPack { /// create an initialized instance from a preset static std::pair create( - const SimpleConfPreset& preset) { - // allocate chemistry_data and set the defaults - std::unique_ptr my_chem(new chemistry_data); - if (local_initialize_chemistry_parameters(my_chem.get()) != GR_SUCCESS) { - return {GrackleCtxPack(), - error::Adhoc("initialize_chemistry_parameters failed")}; - } - - // lookup the parameters associated with the preset - std::pair, Status> chem_preset_rslt = - get_chem_preset_vals_(preset.chemistry); - if (!chem_preset_rslt.second.is_ok()) { - return {GrackleCtxPack(), chem_preset_rslt.second}; - } - const std::vector& params = chem_preset_rslt.first; - - // update my_chem with values from the preset - param_detail::StrAllocTracker str_allocs; - Status status = - set_params(params.begin(), params.end(), *my_chem, &str_allocs); - if (!status.is_ok()) { - return {GrackleCtxPack(), status}; - } - - // set up chemistry_data_storage - code_units initial_unit = setup_initial_unit(preset.unit); - std::unique_ptr my_rates( - new chemistry_data_storage); - if (local_initialize_chemistry_data(my_chem.get(), my_rates.get(), - &initial_unit) != GR_SUCCESS) { - return {GrackleCtxPack(), - error::Adhoc("initialize_chemistry_data failed")}; - } - - std::pair out{GrackleCtxPack(), OkStatus()}; - out.first.initial_units_ = initial_unit; - out.first.str_allocs_ = std::move(str_allocs); - out.first.my_chemistry_ = std::move(my_chem); - out.first.my_rates_ = std::move(my_rates); - return out; - } + const SimpleConfPreset& preset); }; } // namespace grtest From 9f248fbff3c701fe09db15d897202cc854043083 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 24 Jan 2026 19:16:30 -0500 Subject: [PATCH 09/20] Replace SimpleConfPreset with ParamConf --- tests/grtestutils/GrackleCtxPack.cpp | 24 +++- tests/grtestutils/GrackleCtxPack.hpp | 5 +- tests/grtestutils/googletest/fixtures.hpp | 7 +- tests/grtestutils/param.cpp | 5 +- tests/grtestutils/param.hpp | 9 +- tests/grtestutils/preset.cpp | 31 ++++- tests/grtestutils/preset.hpp | 148 +++++++++++++++++++++- tests/unit/test_api_ratequery.cpp | 22 ++-- tests/unit/test_ghost_zone.cpp | 15 ++- 9 files changed, 222 insertions(+), 44 deletions(-) diff --git a/tests/grtestutils/GrackleCtxPack.cpp b/tests/grtestutils/GrackleCtxPack.cpp index 308fb91e5..487bc96b7 100644 --- a/tests/grtestutils/GrackleCtxPack.cpp +++ b/tests/grtestutils/GrackleCtxPack.cpp @@ -10,12 +10,13 @@ /// //===----------------------------------------------------------------------===// -#include "GrackleCtxPack.hpp" +#include "./GrackleCtxPack.hpp" +#include "./preset.hpp" namespace grtest { std::pair GrackleCtxPack::create( - const SimpleConfPreset& preset) { + const ParamConf& param_conf) { // allocate chemistry_data and set the defaults std::unique_ptr my_chem(new chemistry_data); if (local_initialize_chemistry_parameters(my_chem.get()) != GR_SUCCESS) { @@ -25,22 +26,31 @@ std::pair GrackleCtxPack::create( // lookup the parameters associated with the preset std::pair, Status> chem_preset_rslt = - get_chem_preset_vals_(preset.chemistry); - if (!chem_preset_rslt.second.is_ok()) { + get_chem_preset_vals_(param_conf.chem_preset()); + if (chem_preset_rslt.second.is_err()) { return {GrackleCtxPack(), chem_preset_rslt.second}; } const std::vector& params = chem_preset_rslt.first; - // update my_chem with values from the preset + // initialize my_chem's string storage param_detail::StrAllocTracker str_allocs; + + // update my_chem with values from the preset Status status = set_params(params.begin(), params.end(), *my_chem, &str_allocs); - if (!status.is_ok()) { + if (status.is_err()) { + return {GrackleCtxPack(), status}; + } + + // update my_chem with values from parameter overrides (if there are any) + const std::vector& po_vec = param_conf.param_overrides(); + status = set_params(po_vec.begin(), po_vec.end(), *my_chem, &str_allocs); + if (status.is_err()) { return {GrackleCtxPack(), status}; } // set up chemistry_data_storage - code_units initial_unit = setup_initial_unit(preset.unit); + code_units initial_unit = setup_initial_unit(param_conf.unit_preset()); std::unique_ptr my_rates(new chemistry_data_storage); if (local_initialize_chemistry_data(my_chem.get(), my_rates.get(), &initial_unit) != GR_SUCCESS) { diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index f086ad636..02cbef659 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -115,9 +115,8 @@ class GrackleCtxPack { chemistry_data* my_chemistry() { return this->my_chemistry_.get(); } chemistry_data_storage* my_rates() { return this->my_rates_.get(); } - /// create an initialized instance from a preset - static std::pair create( - const SimpleConfPreset& preset); + /// create an initialized instance + static std::pair create(const ParamConf& param_conf); }; } // namespace grtest diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index 00e5270f0..56ebd140d 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -45,9 +45,8 @@ class ConfigPresetFixture : public testing::Test { protected: void SetUp() override { // called immediately after the constructor (but before the test-case) - - std::pair tmp = - GrackleCtxPack::create(SimpleConfPreset{chem_preset, unit_preset}); + ParamConf conf = ParamConf::SimplePreset(chem_preset, unit_preset); + std::pair tmp = GrackleCtxPack::create(conf); if (tmp.second.is_ok()) { pack = std::move(tmp.first); } else if (tmp.second.is_missing_std_file()) { @@ -74,7 +73,7 @@ class ConfigPresetFixture : public testing::Test { /// this class, OR /// 2. make a subclass, named `MyFeatureTest`, of this class class ParametrizedConfigPresetFixture - : public testing::TestWithParam { + : public testing::TestWithParam { protected: void SetUp() override { // called immediately after the constructor (but before the test-case) diff --git a/tests/grtestutils/param.cpp b/tests/grtestutils/param.cpp index 054262152..7feb0de1a 100644 --- a/tests/grtestutils/param.cpp +++ b/tests/grtestutils/param.cpp @@ -26,7 +26,7 @@ constexpr std::false_type always_false_{}; namespace grtest { -std::string ParamVal::to_string() const { +std::string ParamVal::to_string(bool unwrap) const { std::string tmp; // this lambda function is passed a reference to the value within this->val_ auto get_string_repr_ = [&tmp](const auto& v) -> void { @@ -46,6 +46,9 @@ std::string ParamVal::to_string() const { }; std::visit(get_string_repr_, this->val_); + if (unwrap) { + return tmp; + } std::string out; out.reserve(tmp.size() + 10); out = "ParamVal("; diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/param.hpp index 50af101f1..a45881743 100644 --- a/tests/grtestutils/param.hpp +++ b/tests/grtestutils/param.hpp @@ -58,7 +58,10 @@ class ParamVal { explicit ParamVal(const char* val) : val_(ParamVal::coerce_c_string_(val)) {} /// return a string-representation of `this` - std::string to_string() const; + /// + /// @param[in] unwrap When true, we just print the inner value. Otherwise, + /// the value is written as `ParamVal()` + std::string to_string(bool unwrap = false) const; /// teach googletest how to print the value friend void PrintTo(const ParamVal& p, std::ostream* os); @@ -97,8 +100,8 @@ class ParamVal { using ParamPair = std::pair; /// Nicely format a @ref ParamPair as a string -inline std::string to_string(const ParamPair& pair) { - std::string str_val = pair.second.to_string(); +inline std::string to_string(const ParamPair& pair, bool unwrap_val = false) { + std::string str_val = pair.second.to_string(unwrap_val); std::string out; out.reserve(pair.first.size() + str_val.size() + 6); out += "{\""; diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index 0152bc0b3..07bca4a06 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -18,10 +18,13 @@ #include "status_reporting.h" // GR_INTERNAL_UNREACHABLE_ERROR #include +#include namespace grtest { -static std::string to_string(const grtest::ChemPreset& preset) { +std::string to_string(const grtest::ChemPreset& preset) { switch (preset) { + case grtest::ChemPreset::DEFAULT: + return ""; case grtest::ChemPreset::primchem0: return "pc=0"; case grtest::ChemPreset::primchem1: @@ -37,8 +40,16 @@ static std::string to_string(const grtest::ChemPreset& preset) { GR_INTERNAL_UNREACHABLE_ERROR(); } +void PrintTo(const grtest::ChemPreset& chem_preset, std::ostream* os) { + *os << to_string(chem_preset); +} + std::pair, Status> get_chem_preset_vals_( ChemPreset preset) { + if (preset == ChemPreset::DEFAULT) { + return {{}, OkStatus()}; + } + std::vector> v = make_ParamPair_vec({ {"use_grackle", 1}, // chemistry on {"use_isrf_field", 1}, @@ -56,6 +67,10 @@ std::pair, Status> get_chem_preset_vals_( } switch (preset) { + case ChemPreset::DEFAULT: { + // we should have returned before now + GR_INTERNAL_UNREACHABLE_ERROR(); + } case ChemPreset::primchem0: { v.emplace_back("primordial_chemistry", 0); v.emplace_back("dust_chemistry", 0); @@ -88,7 +103,7 @@ std::pair, Status> get_chem_preset_vals_( GR_INTERNAL_UNREACHABLE_ERROR(); } -static std::string to_string(const InitialUnitPreset& preset) { +std::string to_string(const InitialUnitPreset& preset) { switch (preset) { case grtest::InitialUnitPreset::simple_z0: return "simpleUnit-z=0"; @@ -117,9 +132,15 @@ code_units setup_initial_unit(InitialUnitPreset preset) { GR_INTERNAL_UNREACHABLE_ERROR(); } -void PrintTo(const grtest::SimpleConfPreset& preset, std::ostream* os) { - *os << "Preset{" << to_string(preset.chemistry) << ',' - << to_string(preset.unit) << '}'; +void PrintTo(const ParamConf& param_conf, std::ostream* os) { + param_conf.print_descr(os, true); +} + +std::string ParamConf::stringify(bool short_summary, + const std::string& common_indent) const { + std::ostringstream s; + this->print_descr(&s, short_summary, common_indent); + return s.str(); } } // namespace grtest diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index 808f3c52e..23de0937f 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -14,7 +14,6 @@ #include "grackle.h" #include "./param.hpp" -#include "./status.hpp" #include #include @@ -25,6 +24,7 @@ namespace grtest { /// @note /// In the future, we probably want to add more enum class ChemPreset { + DEFAULT, primchem0, primchem1, primchem2, @@ -32,6 +32,10 @@ enum class ChemPreset { primchem4_dustspecies3, }; +void PrintTo(const ChemPreset& chem_preset, std::ostream* os); + +std::string to_string(const ChemPreset& preset); + /// override the settings of my_chem based on the specified preset std::pair, Status> get_chem_preset_vals_( ChemPreset preset); @@ -45,17 +49,147 @@ enum class InitialUnitPreset { simple_z0, // <- no cosmology, z=0 }; +std::string to_string(const InitialUnitPreset& preset); + /// return a code_unit instance initialized based on the specified preset code_units setup_initial_unit(InitialUnitPreset preset); -/// Represents the preset for creating a GrackleCtxPack -struct SimpleConfPreset { - ChemPreset chemistry; - InitialUnitPreset unit; +/// Represents a configuration for creating a GrackleCtxPack with overrides +/// +/// @note +/// This type is fleshed out a class, rather than just being a simpler +/// struct-like type for the sole purpose of providing nice stringification +/// operations +class ParamConf { + // this whole class is a little clunky... + + /// the chemistry preset + ChemPreset c_preset_; + /// the initial unit preset + InitialUnitPreset u_preset_; + + // A few thoughts about overrides_: + // 1. it's probably better if it's some kind of mapping type + // 2. long term, we may want overrides to be able to override options from + // u_preset_ + + /// the parameter overrides + std::vector overrides_; + + // it's only possible to default construct through a factory method + ParamConf() = default; + + bool is_overridden_(std::string_view name) const { + for (const ParamPair& pair : overrides_) { + if (name == pair.first) { + return true; + } + } + return false; + } + +public: + /// Represents a ParamConfig that is simply built from presets + static ParamConf SimplePreset(ChemPreset chemistry, InitialUnitPreset unit) { + ParamConf out; + out.c_preset_ = chemistry; + out.u_preset_ = unit; + return out; + } + + ParamConf(const std::optional& chemistry, InitialUnitPreset unit, + const std::vector& param_overrides) + : c_preset_(chemistry.value_or(ChemPreset::DEFAULT)), + u_preset_{unit}, + overrides_(param_overrides) {} + + ParamConf(const std::optional& chemistry, InitialUnitPreset unit, + std::vector&& param_overrides) + : c_preset_(chemistry.value_or(ChemPreset::DEFAULT)), + u_preset_{unit}, + overrides_(std::move(param_overrides)) {} + + ChemPreset chem_preset() const { return c_preset_; } + const std::vector& param_overrides() const { return overrides_; } + InitialUnitPreset unit_preset() const { return u_preset_; } + + /// teach Googletest to print the simple description + friend void PrintTo(const ParamConf& param_conf, std::ostream* os); + + /// this is used to help describe the parameters used in a test result message + /// + /// @note + /// This accepts a template parameter Stream because we want this to work + /// with Googletest's `::testing::AssertionResult` type. While that type + /// is probably a subclass of `std::ostream`, that's not explicitly stated in + /// the documentation (and I don't want things to break) + template + void print_descr(Sink* s, bool short_descr, + const std::string& common_indent = "") const; + + /// prints a simple description + std::string stringify(bool short_summary, + const std::string& common_indent = "") const; }; -// teach googletest how to print SimpleConfPreset -void PrintTo(const SimpleConfPreset& preset, std::ostream* os); +template +void ParamConf::print_descr(Sink* s, bool short_descr, + const std::string& common_indent) const { + std::string c_p_str = to_string(this->c_preset_); + std::string u_p_str = to_string(this->u_preset_); + + std::size_t n_override = this->overrides_.size(); + + if (short_descr) { + if (n_override == 0) { + *s << common_indent << "Preset{"; + *s << c_p_str << ',' << to_string(this->u_preset_) << '}'; + } else { + *s << common_indent << "ParamConf("; + *s << c_p_str << ",<+" << n_override << "overrides>," << u_p_str << ')'; + } + return; + } + + // the rest of this function is dedicated to providing a detailed summary + std::string level1_indent = common_indent + " "; + std::string level2_indent = common_indent + " "; + + *s << common_indent << "ParamConf{\n"; + *s << level1_indent << "ChemPreset: " << c_p_str << ",\n"; + + // Show non-overridden parameters corresponding to chem preset + if (this->c_preset_ != ChemPreset::DEFAULT) { + *s << level1_indent << "Non-Overridden Preset Params: "; + std::pair, Status> tmp = + get_chem_preset_vals_(this->c_preset_); + if (tmp.second.is_err()) { + *s << "\n"; + } else { + // record the pair isn't overridden + *s << "{\n"; + for (const ParamPair& pair : tmp.first) { + if (!this->is_overridden_(pair.first)) { + *s << level2_indent << to_string(pair, true) << ",\n"; + } + } + *s << level1_indent << "},\n"; + } + } + + // handle details about the overrides + *s << level1_indent << "Parameter Overrides: {\n"; + for (const ParamPair& pair : this->overrides_) { + *s << level2_indent << to_string(pair, true) << ",\n"; + } + *s << level1_indent << "},\n"; + + // finally show the unit-preset + *s << level1_indent << "Unit Preset: " << u_p_str << '\n'; + + // close the brace + *s << common_indent << "}\n"; +} } // namespace grtest diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index f163f192a..07ef9ee85 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -79,14 +79,20 @@ TEST_P(ParametrizedRateQueryTest, ConsistentIDs) { using grtest::ChemPreset; using grtest::InitialUnitPreset; -using grtest::SimpleConfPreset; +using grtest::ParamConf; + +static const ParamConf my_presets_[] = { + ParamConf::SimplePreset(ChemPreset::primchem0, + InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem1, + InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem2, + InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem3, + InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem4_dustspecies3, + InitialUnitPreset::simple_z0)}; INSTANTIATE_TEST_SUITE_P( /* 1st arg is intentionally empty */, ParametrizedRateQueryTest, - ::testing::Values( - SimpleConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem2, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem4_dustspecies3, - InitialUnitPreset::simple_z0})); + ::testing::ValuesIn(my_presets_)); diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index 317b055b7..e06f5f6b9 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -342,15 +342,18 @@ TEST_P(APIGhostZoneTest, GridZoneStartEnd) { } -using grtest::SimpleConfPreset; +using grtest::ParamConf; using grtest::ChemPreset; using grtest::InitialUnitPreset; +static const ParamConf my_presets_[] = { + ParamConf::SimplePreset(ChemPreset::primchem0, InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem1, InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem2, InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem3, InitialUnitPreset::simple_z0) +}; + INSTANTIATE_TEST_SUITE_P( /* 1st arg is intentionally empty */, APIGhostZoneTest, - ::testing::Values( - SimpleConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem2, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}) + ::testing::ValuesIn(my_presets_) ); From 9e4c7dd5599ddd72a608ec473faf9231eb162832 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 24 Jan 2026 19:31:40 -0500 Subject: [PATCH 10/20] introduce GRTest_MAKE_CTX_PACK --- tests/grtestutils/googletest/fixtures.hpp | 93 +++++++++++++++++++---- 1 file changed, 77 insertions(+), 16 deletions(-) diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index 56ebd140d..db7685d53 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -24,6 +24,80 @@ #include // std::move +// C Pre-Processor weirdness requires an additional level of indirection +// if we want B to be passed __LINE__ and actually expand to a line number +#define GRTest_CONCAT_TOKEN_HELPER_(a, b) a##b +#define GRTest_CONCAT_TOKEN_(a, b) GRTest_CONCAT_TOKEN_HELPER_(a, b) + +/// Construct a GrackleCtxPack with googletest error checking +/// +/// In more detail, this calls `GrackleCtxPack::create(conf)`, performs +/// associated error-checking and then stores the result in `lhs`. This is +/// roughly analogous to +/// \code{C++} +/// RsltPairT rslt_pair = ::grtest::GrackleCtxPack::create(conf); +/// if (!rslt_pair.second.is_err()) { +/// /* handle the error with Googletest machinery */ +/// } +/// lhs = std::move(rslt_pair.first); +/// \endcode +/// In the above pseudocode, 2 simplifications were made: +/// - `RsltPairT` replaced std::pair<::grtest::GrackleCtxPack, ::grtest::Status> +/// - the variable name `rslt_pair` replaces a more unique variable name +/// +/// @param[in, out] lhs If construction of the pack succeeds, then this will +/// be on the left hand side of the final assignment expression +/// @param[in] conf Expands to eSimpleConfPreset +/// +/// @par Motivation +/// This exists so that one-off test-cases don't need to create a full-blown +/// fixture to check a given configuration (they can use this macro instead) +/// +/// @warning +/// Because the error-checking logic may trigger skipping of a test or a test +/// failure, this **MUST** be called within the body of a test-case or in the +/// `SetUp()`/`TearDown()` method of a fixture. In other words, **DO NOT** call +/// this in a helper function. For more context, see: +/// https://google.github.io/googletest/advanced.html#assertion-placement +/// +/// @par Broader Thoughts +/// I really don't like this. But, it currently seems like the most pragmatic +/// solution for now... +/// - Part of the issue is that I have ambitions to reuse the GrackleCtxPack +/// for benchmarking and possibly fuzz/property testing and I didn't want to +/// pigeonhole ourselves... +/// - Now that error-handling of `GrackleCtxPack::create` has improved, and we +/// can provide a detailed description as to why construction failed (without +/// writing much extra code), I'm starting to think that we should replace +/// this with a simple function that either returns a fully-constructed +/// GrackleCtxPack or aborts the program +/// - notably, this choice means that this gets rid of test-skipping +/// functionallity. +/// - I think the fact that this solution would abort the program rather than +/// gracefully reporting a problem is probably ok... Since error-reporting +/// is more robust, it would probably be straight-forward to write +/// alternative functions with specialized handling on a case-by-case basis +/// (e.g. for tests that check Grackle's behavior for deliberately invalid +/// configuration options) +/// - I'm inclined to stick with this macro for now, until the better handling +/// approach becomes a little more obvious... +#define GRTest_MAKE_CTX_PACK(lhs, conf) \ + /* GRTest_CONCAT_TOK(rslt, __LINE__) is the temporary variable's name */ \ + std::pair<::grtest::GrackleCtxPack, ::grtest::Status> GRTest_CONCAT_TOKEN_( \ + rslt, __LINE__) = ::grtest::GrackleCtxPack::create(conf); \ + \ + /* Check whether construction succeeded */ \ + if (GRTest_CONCAT_TOKEN_(rslt, __LINE__).second.is_missing_std_file()) { \ + GTEST_SKIP() << GRTest_CONCAT_TOKEN_(rslt, __LINE__).second.to_string(); \ + } else if (GRTest_CONCAT_TOKEN_(rslt, __LINE__).second.is_err()) { \ + FAIL() << GRTest_CONCAT_TOKEN_(rslt, __LINE__).second.to_string() \ + << " occurred for:\n" \ + << conf.stringify(false, " "); \ + } \ + \ + /* store the result in lhs */ \ + lhs = std::move(GRTest_CONCAT_TOKEN_(rslt, __LINE__).first) + namespace grtest { /// test-fixture that can used to run one or more tests with a chemistry data @@ -46,14 +120,7 @@ class ConfigPresetFixture : public testing::Test { void SetUp() override { // called immediately after the constructor (but before the test-case) ParamConf conf = ParamConf::SimplePreset(chem_preset, unit_preset); - std::pair tmp = GrackleCtxPack::create(conf); - if (tmp.second.is_ok()) { - pack = std::move(tmp.first); - } else if (tmp.second.is_missing_std_file()) { - GTEST_SKIP() << tmp.second.to_string(); - } else { - FAIL() << tmp.second.to_string(); - } + GRTest_MAKE_CTX_PACK(pack, conf); } GrackleCtxPack pack; @@ -77,14 +144,8 @@ class ParametrizedConfigPresetFixture protected: void SetUp() override { // called immediately after the constructor (but before the test-case) - std::pair tmp = GrackleCtxPack::create(GetParam()); - if (tmp.second.is_ok()) { - pack = std::move(tmp.first); - } else if (tmp.second.is_missing_std_file()) { - GTEST_SKIP() << tmp.second.to_string(); - } else { - FAIL() << tmp.second.to_string(); - } + ParamConf conf = GetParam(); + GRTest_MAKE_CTX_PACK(pack, conf); } GrackleCtxPack pack; From f56e7df8bbff69a563412d06843743fcfd470db3 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Wed, 4 Feb 2026 23:18:40 -0500 Subject: [PATCH 11/20] minor bugfix --- tests/grtestutils/param.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/param.hpp index a45881743..55ddc224f 100644 --- a/tests/grtestutils/param.hpp +++ b/tests/grtestutils/param.hpp @@ -14,6 +14,7 @@ #include "grackle.h" #include "./status.hpp" +#include // std::nullptr_t #include #include #include @@ -37,7 +38,7 @@ class ParamVal { const ParamVal& par_val, param_detail::StrAllocTracker* str_allocs); - using val_type = std::variant; + using val_type = std::variant; // private attribute val_type val_; From fa8852c5ec96c8f2b263f293dbbe1bd3a5727829 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sun, 1 Feb 2026 23:32:22 -0500 Subject: [PATCH 12/20] adjust preset --- tests/grtestutils/preset.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index 07bca4a06..81b072134 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -51,8 +51,7 @@ std::pair, Status> get_chem_preset_vals_( } std::vector> v = make_ParamPair_vec({ - {"use_grackle", 1}, // chemistry on - {"use_isrf_field", 1}, + {"use_grackle", 1}, // chemistry on {"with_radiative_cooling", 1}, // cooling on {"metal_cooling", 1}, // metal cooling on {"UVbackground", 1}, // UV background on From f2e7ec0973f04aa6278a8cb5175aa3dfba887018 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Wed, 4 Feb 2026 23:44:19 -0500 Subject: [PATCH 13/20] another minor bugfix --- tests/grtestutils/param.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/grtestutils/param.cpp b/tests/grtestutils/param.cpp index 7feb0de1a..e6834054c 100644 --- a/tests/grtestutils/param.cpp +++ b/tests/grtestutils/param.cpp @@ -74,7 +74,7 @@ bool ParamVal::is_equal(std::string_view val) const { bool ParamVal::is_equal(const char* val) const { if (val == nullptr) { - return std::holds_alternative(val_); + return std::holds_alternative(val_); } return is_equal(std::string_view(val)); } From db2693d1e8c761fcfe1334f85a3ed63dc8d8718c Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 5 Feb 2026 00:14:41 -0500 Subject: [PATCH 14/20] hopefully this is the last fix --- tests/grtestutils/param.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/param.hpp index 55ddc224f..90473e8d6 100644 --- a/tests/grtestutils/param.hpp +++ b/tests/grtestutils/param.hpp @@ -85,7 +85,7 @@ class ParamVal { /// Returns whether `this` holds an empty string bool is_empty_string() const { - return std::holds_alternative(val_); + return std::holds_alternative(val_); } /*!{*/ From 207e3412e3e1274f4678f614aa3f01da8c774c73 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 12 Feb 2026 20:28:19 -0500 Subject: [PATCH 15/20] rearranging files --- tests/grtestutils/CMakeLists.txt | 9 +++++---- tests/grtestutils/googletest/fixtures.hpp | 6 +++--- .../grackle_ctx_pack.cpp} | 2 +- .../grackle_ctx_pack.hpp} | 0 tests/grtestutils/{ => harness}/param.cpp | 11 ++++++----- tests/grtestutils/{ => harness}/param.hpp | 0 tests/grtestutils/{ => harness}/preset.cpp | 2 +- tests/grtestutils/{ => harness}/preset.hpp | 0 tests/grtestutils/{ => harness}/status.cpp | 0 tests/grtestutils/{ => harness}/status.hpp | 0 tests/grtestutils/iterator_adaptor.hpp | 3 ++- 11 files changed, 18 insertions(+), 15 deletions(-) rename tests/grtestutils/{GrackleCtxPack.cpp => harness/grackle_ctx_pack.cpp} (98%) rename tests/grtestutils/{GrackleCtxPack.hpp => harness/grackle_ctx_pack.hpp} (100%) rename tests/grtestutils/{ => harness}/param.cpp (95%) rename tests/grtestutils/{ => harness}/param.hpp (100%) rename tests/grtestutils/{ => harness}/preset.cpp (99%) rename tests/grtestutils/{ => harness}/preset.hpp (100%) rename tests/grtestutils/{ => harness}/status.cpp (100%) rename tests/grtestutils/{ => harness}/status.hpp (100%) diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index 3cd85dc2b..ca218faa1 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -80,14 +80,15 @@ add_library(grtestutils # files outside of the googletest subdirectory # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp - GrackleCtxPack.cpp GrackleCtxPack.hpp iterator_adaptor.hpp os.hpp os.cpp - param.cpp param.hpp - preset.cpp preset.hpp - status.cpp status.hpp utils.hpp utils.cpp + harness/grackle_ctx_pack.cpp harness/grackle_ctx_pack.hpp + harness/param.cpp harness/param.hpp + harness/preset.cpp harness/preset.hpp + harness/status.cpp harness/status.hpp + # files in the googletest subdirectory (reminder: should just be headers!) googletest/check_allclose.hpp googletest/fixtures.hpp diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index db7685d53..4886e0a29 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -18,9 +18,9 @@ // because we include gtest.h here, we should NOT include this file in any // grtest source files (in other words, this should be a header-only file) -#include "../GrackleCtxPack.hpp" -#include "../preset.hpp" -#include "../status.hpp" +#include "../harness/grackle_ctx_pack.hpp" +#include "../harness/preset.hpp" +#include "../harness/status.hpp" #include // std::move diff --git a/tests/grtestutils/GrackleCtxPack.cpp b/tests/grtestutils/harness/grackle_ctx_pack.cpp similarity index 98% rename from tests/grtestutils/GrackleCtxPack.cpp rename to tests/grtestutils/harness/grackle_ctx_pack.cpp index 487bc96b7..e135d298c 100644 --- a/tests/grtestutils/GrackleCtxPack.cpp +++ b/tests/grtestutils/harness/grackle_ctx_pack.cpp @@ -10,7 +10,7 @@ /// //===----------------------------------------------------------------------===// -#include "./GrackleCtxPack.hpp" +#include "./grackle_ctx_pack.hpp" #include "./preset.hpp" namespace grtest { diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/harness/grackle_ctx_pack.hpp similarity index 100% rename from tests/grtestutils/GrackleCtxPack.hpp rename to tests/grtestutils/harness/grackle_ctx_pack.hpp diff --git a/tests/grtestutils/param.cpp b/tests/grtestutils/harness/param.cpp similarity index 95% rename from tests/grtestutils/param.cpp rename to tests/grtestutils/harness/param.cpp index e6834054c..3eb293ec8 100644 --- a/tests/grtestutils/param.cpp +++ b/tests/grtestutils/harness/param.cpp @@ -11,11 +11,12 @@ //===----------------------------------------------------------------------===// #include "status_reporting.h" -#include "param.hpp" -#include "GrackleCtxPack.hpp" // param_detail::StrAllocTracker -#include // std::strlen -#include // std::ostream -#include // std::false_type, std::is_same_v, std::decay_t +#include "./param.hpp" +#include "./grackle_ctx_pack.hpp" // param_detail::StrAllocTracker + +#include // std::strlen +#include // std::ostream +#include // std::false_type, std::is_same_v, std::decay_t namespace { // stuff inside an anonymous namespace is local to this file // this is used to report a compile-time error in the lambda function that diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/harness/param.hpp similarity index 100% rename from tests/grtestutils/param.hpp rename to tests/grtestutils/harness/param.hpp diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/harness/preset.cpp similarity index 99% rename from tests/grtestutils/preset.cpp rename to tests/grtestutils/harness/preset.cpp index 81b072134..f98a79838 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/harness/preset.cpp @@ -12,7 +12,7 @@ #include "./param.hpp" #include "./preset.hpp" -#include "./utils.hpp" +#include "../utils.hpp" #include "grackle.h" #include "status_reporting.h" // GR_INTERNAL_UNREACHABLE_ERROR diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/harness/preset.hpp similarity index 100% rename from tests/grtestutils/preset.hpp rename to tests/grtestutils/harness/preset.hpp diff --git a/tests/grtestutils/status.cpp b/tests/grtestutils/harness/status.cpp similarity index 100% rename from tests/grtestutils/status.cpp rename to tests/grtestutils/harness/status.cpp diff --git a/tests/grtestutils/status.hpp b/tests/grtestutils/harness/status.hpp similarity index 100% rename from tests/grtestutils/status.hpp rename to tests/grtestutils/harness/status.hpp diff --git a/tests/grtestutils/iterator_adaptor.hpp b/tests/grtestutils/iterator_adaptor.hpp index c992b9490..ddb510558 100644 --- a/tests/grtestutils/iterator_adaptor.hpp +++ b/tests/grtestutils/iterator_adaptor.hpp @@ -16,7 +16,8 @@ #include #include "grackle.h" -#include "GrackleCtxPack.hpp" + +#include "./harness/grackle_ctx_pack.hpp" namespace grtest { From 5d5ec6d5ffc728771da5d3346884f7c1cb18b5f8 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 20 Feb 2026 10:26:18 -0500 Subject: [PATCH 16/20] introduce grtest::IdxMapping --- tests/grtestutils/view.hpp | 278 +++++++++++++++++++++++++++++++++++++ 1 file changed, 278 insertions(+) diff --git a/tests/grtestutils/view.hpp b/tests/grtestutils/view.hpp index 88d03ec12..102cee1fc 100644 --- a/tests/grtestutils/view.hpp +++ b/tests/grtestutils/view.hpp @@ -16,11 +16,275 @@ #ifndef GRTESTUTILS_VIEW_HPP #define GRTESTUTILS_VIEW_HPP +#include #include // std::size_t +#include +#include #include +#include // std::pair + +#include "status_reporting.h" namespace grtest { +/// To be used with @ref IdxMapping +enum struct DataLayout { + LEFT, ///< the leftmost dimension has a stride 1 + RIGHT ///< the rightmost dimension has a stride 1 +}; + +/// Maps multi-dimensional indices to a 1D pointer offset +/// +/// Broader Context +/// =============== +/// To best describe this type, it's insightful to draw comparisons with C++ +/// conventions. +/// +/// Background +/// ---------- +/// For some background, C++23 introduced `std::mdspan` to describe +/// multi-dimensional views. A `std::mdspan` is parameterized by +/// - the data's extents (aka the shape) +/// - the data's layout, which dictates how a multidimensional index is mapped +/// to a 1D pointer offset +/// +/// For views of contiguous data there are 2 obvious layouts: +/// 1. layout-right: where the stride is `1` along the rightmost extent. +/// - for extents `{a,b,c}`, an optimal nested for-loop will iterates from +/// `0` up to `a` in the outermost loop and from `0` up to `c` +/// in the innermost loop +/// - this is the "natural layout" for a multidimensional c-style array +/// `arr[a][b][c]` +/// 2. layout-left: where the stride is `1` along the leftmost extent +/// - for extents `{a,b,c}`, an optimal nested for-loop will iterates from +/// `0` up to `c` in the outermost loop and from `0` up to `a` +/// in the innermost loop +/// - this is the "natural layout" for a multidimensional fortran array +/// `arr[a][b][c]` +/// +/// > Aside: More sophisticated layouts are possible when strides along each +/// > axis aren't directly tied to extents (this comes up when making subviews). +/// +/// About this type +/// --------------- +/// This type specifies the data layout, extents, and provides the mapping. You +/// draw analogies with C++23 types: +/// - `IdxMapping` <---> `std::layout_left::mapping` +/// - `IdxMapping` <---> `std::layout_right::mapping` +/// +/// At the time of writing, the type **only** represents contiguous data layouts +/// +/// @note +/// The template specialization using @ref DataLayout::RIGHT mostly exists for +/// exposition purposes +/// - it's useful to talk about this scenario since it is the "natural" layout +/// for C and C++ +/// - in practice, Grackle was written assuming @ref DataLayout::LEFT. Thus, +/// most loops will initially get written assuming that layout (but we always +/// have the option to conditionally provide the better kind of loop) +template +struct IdxMapping { + static_assert(Layout == DataLayout::LEFT || Layout == DataLayout::RIGHT, + "A new layout type was introduced that we don't yet support"); + static constexpr DataLayout layout = Layout; + + /// max rank value + static constexpr int MAX_RANK = 3; + +private: + // attributes: + int rank_; ///< the number of dimensions + int extents_[MAX_RANK]; ///< dimensions of the multi-dimensional index-space + + // private functions: + + // default constructor is only invoked by factory methods + // -> this explicitly set each extent_ to a value of 0 + IdxMapping() : rank_(0), extents_{} {} + + // the object returned by the create_ factory method: + // - `out.second == nullptr` indicates the method succeeded + // - otherwise, `out.second` is a string-literal specifying an error message + using InnerMappingMsgPair_ = std::pair, const char*>; + + static InnerMappingMsgPair_ create_(int rank, const int* extents) noexcept { + // arg checking + if ((rank < 1) || (rank > MAX_RANK)) { + return {IdxMapping(), "rank is invalid"}; + } else if (extents == nullptr) { + return {IdxMapping(), "extents is a nullptr"}; + } + for (int i = 0; i < rank; i++) { + if (extents[i] < 1) { + return {IdxMapping(), "extents must hold positive vals"}; + } + } + + // build and return the mapping + IdxMapping mapping; + mapping.rank_ = rank; + for (int i = 0; i < rank; i++) { + mapping.extents_[i] = extents[i]; + } + return {mapping, nullptr}; + } + + /// factory method that aborts with an error message upon failure + static IdxMapping create_or_abort_(int rank, const int* extents) { + InnerMappingMsgPair_ pair = IdxMapping::create_(rank, extents); + if (pair.second != nullptr) { + GR_INTERNAL_ERROR("%s", pair.second); + } + return pair.first; + } + +public: + /// factory method that returns an empty optional upon failure + static std::optional> try_create(int rank, + const int* extents) { + InnerMappingMsgPair_ pair = IdxMapping::create_(rank, extents); + return (pair.second == nullptr) + ? std::optional>{pair.first} + : std::optional>{}; + } + + explicit IdxMapping(int extent0) { + // for less experienced C++ devs: the `explicit` kwarg prevents the use of + // this constructor for implicit casts + int extents[1] = {extent0}; + *this = create_or_abort_(1, extents); + } + + IdxMapping(int extent0, int extent1) { + int extents[2] = {extent0, extent1}; + *this = create_or_abort_(2, extents); + } + + IdxMapping(int extent0, int extent1, int extent2) { + int extents[3] = {extent0, extent1, extent2}; + *this = create_or_abort_(3, extents); + } + + IdxMapping(const IdxMapping&) = default; + IdxMapping(IdxMapping&&) = default; + IdxMapping& operator=(const IdxMapping&) = default; + IdxMapping& operator=(IdxMapping&&) = default; + ~IdxMapping() = default; + + /// access the rank + int rank() const noexcept { return rank_; } + + /// access the extents pointer (the length is given by @ref rank) + const int* extents() const noexcept { return extents_; } + + /// construct an equivalent 3d IdxMapping + IdxMapping to_3d_mapping() const noexcept { + int out_rank = 3; + int rank_diff = out_rank - this->rank_; + GR_INTERNAL_REQUIRE(rank_diff >= 0, "current rank exceeds new rank"); + + IdxMapping out; + out.rank_ = out_rank; + for (int i = 0; i < out_rank; i++) { + out.extents_[i] = 1; + } + int offset = (Layout == DataLayout::RIGHT) ? rank_diff : 0; + for (int i = out; i < this->rank_; i++) { + out.extents_[i + offset] = this->extents_[i]; + } + return out; + } + + static constexpr bool is_contiguous() { return true; } + + int n_elements() const noexcept { + int product = 1; + for (int i = 0; i < this->rank_; i++) { + product *= this->extents_[i]; + } + return product; + } + + /** @{ */ // <- open the group of member functions with a shared docstring + /// compute the 1D pointer offset associated with the multidimensional index + /// + /// @note + /// For less experienced C++ devs, these methods overloads the "function call + /// operator". In python they would be named `__call__(self, ...)` + /// + /// Behavior is undefined if the number of arguments doesn't match the value + /// returned by `this->rank()` + [[gnu::always_inline]] int operator()(int i) const noexcept { + assert(this->rank_ == 1); + return i; + } + + [[gnu::always_inline]] int operator()(int i, int j) const noexcept { + assert(this->rank_ == 2); + if constexpr (Layout == DataLayout::LEFT) { + return i + this->extents_[0] * j; + } else { // Layout == DataLayout::RIGHT + return j + this->extents_[1] * i; + } + } + + [[gnu::always_inline]] int operator()(int i, int j, int k) const noexcept { + assert(this->rank_ == 3); + if constexpr (Layout == DataLayout::LEFT) { + return i + this->extents_[0] * (j + k * this->extents_[1]); + } else { // Layout == DataLayout::RIGHT + return k + this->extents_[2] * (j + i * this->extents_[1]); + } + } + /** @} */ // <- close the group of member functions with a shared docstring + + /// Convert a 1D pointer offset to a multidimensional index + /// + /// The number of components for the output index is given by calling the + /// @ref rank method. Behavior is undefined if @p out doesn't have enough + /// space. + /// + /// @param[in] offset The 1D pointer offset (must be non-negative) + /// @param[out] out Buffer where result is stored + void offset_to_md_idx(int offset, int* out) const noexcept { + assert(out != nullptr); + + const int contig_ax = (Layout == DataLayout::LEFT) ? 0 : (this->rank_ - 1); + const int slowest_ax = (Layout == DataLayout::LEFT) ? (this->rank_ - 1) : 0; + + switch (this->rank_) { + case 1: + out[0] = offset; + return; + case 2: + out[slowest_ax] = offset / this->extents_[contig_ax]; + out[contig_ax] = offset % this->extents_[contig_ax]; + return; + case 3: { + int largest_stride = this->extents_[contig_ax] * this->extents_[1]; + out[slowest_ax] = offset / largest_stride; + int remainder = offset % largest_stride; + out[1] = remainder / this->extents_[contig_ax]; + out[contig_ax] = remainder % this->extents_[contig_ax]; + return; + } + default: + GR_INTERNAL_ERROR("should be unreachable"); + } + } + + // teach googletest how to print this type + friend void PrintTo(const IdxMapping& mapping, std::ostream* os) { + const char* layout = + (Layout == DataLayout::LEFT) ? "DataLayout::LEFT" : "DataLayout::RIGHT"; + *os << "IdxMapping<" << layout << ">("; + for (int i = 0; i < mapping.rank_; i++) { + *os << mapping.extents_[i]; + *os << ((i + 1) < mapping.rank_ ? ',' : ')'); + } + } +}; + /// equivalent of converting output of printf("%g", val) to std::string std::string to_pretty_string(float val); std::string to_pretty_string(double val); @@ -29,6 +293,20 @@ std::string to_pretty_string(double val); std::string ptr_to_string(const float* ptr, std::size_t len); std::string ptr_to_string(const double* ptr, std::size_t len); +/// formats a pointer as a string (current implementation prints a 1D array) +template +std::string ptr_to_string(const float* ptr, IdxMapping idx_mapping) { + static_assert(IdxMapping::is_contiguous()); + return ptr_to_string(ptr, idx_mapping.n_elements()); +} + +/// formats a pointer as a string (current implementation prints a 1D array) +template +std::string ptr_to_string(const double* ptr, IdxMapping idx_mapping) { + static_assert(IdxMapping::is_contiguous()); + return ptr_to_string(ptr, idx_mapping.n_elements()); +} + } // namespace grtest #endif // GRTESTUTILS_VIEW_HPP \ No newline at end of file From 0b8b3f3bbc3aeeee93917e723dbc792581264c48 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Mon, 23 Feb 2026 11:21:54 -0500 Subject: [PATCH 17/20] Refactor check_allclose --- .../grtestutils/googletest/check_allclose.cpp | 325 +++++++++++++++--- .../grtestutils/googletest/check_allclose.hpp | 77 ++++- .../googletest/check_allclose_detail.hpp | 143 ++++++++ 3 files changed, 485 insertions(+), 60 deletions(-) create mode 100644 tests/grtestutils/googletest/check_allclose_detail.hpp diff --git a/tests/grtestutils/googletest/check_allclose.cpp b/tests/grtestutils/googletest/check_allclose.cpp index ae6ca702b..d13239ffb 100644 --- a/tests/grtestutils/googletest/check_allclose.cpp +++ b/tests/grtestutils/googletest/check_allclose.cpp @@ -12,81 +12,294 @@ #include "./check_allclose.hpp" #include "../view.hpp" +#include "gtest/gtest.h" +#include "status_reporting.h" #include +#include #include +#include #include #include -testing::AssertionResult check_allclose(const std::vector& actual, - const std::vector& desired, - double rtol, double atol) { - if (actual.size() != desired.size()) { - return testing::AssertionFailure() - << "the compared arrays have different lengths"; +namespace grtest::arraycmp_detail { + +namespace { // stuff inside an anonymous namespace is local to this file + +/// calls the unary function once for each element +/// +/// if selected is provided, we skip locations where selected[i] is false +template +void flat_for_each(Fn fn, int n_elements, const bool* selected) { + if (selected == nullptr) { + for (int flat_idx = 0; flat_idx < n_elements; flat_idx++) { + fn(flat_idx); + } + } else { + for (int flat_idx = 0; flat_idx < n_elements; flat_idx++) { + if (selected[flat_idx]) { + fn(flat_idx); + } + } } +} + +/// Specifies a noteworthy detail about a mismatching pairs of pointers +/// +/// There are 2 kinds of details: +/// - a detail describing a particular pair of mismatching elements (e.g. the +/// very first mismatch, the place where the size of the mismatch is most +/// significant). In these cases, the flattened index is tracked so that +/// the location of the mismatch and the values of the elements can be +/// printed +/// - a generic string that doesn't have any single associated location +struct MismatchDetail { + std::string description; + std::optional flat_idx; + + // the following constructor is defined in order to make this work with + // std::vector::emplace_back. Delete it, once we require C++20 or newer +#if __cpp_aggregate_paren_init < 201902L + MismatchDetail(const std::string& description, + const std::optional& flat_idx) + : description(description), flat_idx(flat_idx) {} +#endif +}; + +/// collects interesting details about mismatched elements in a pair of pointers +/// +/// This is called by @ref compare_ptrs_, if we determine that the compared +/// pointers contain at least one pair of mismatching elements. This loops back +/// over all pairs of elements and collects a vector of noteworthy mismatches. +/// +/// @note +/// It's ok if this is a little slow, as long as it provides useful messages. +/// (After all, this logic only gets invoked when comparisons fail). +template +std::vector collect_details_( + const T* actual, const T* desired, const IdxMapping& idx_mapping, + const bool* selection_mask, Cmp cmp_fn) { + // define some variables that we will fill as we loop over the array + int first_mismatch_idx = -1; + + int first_nan_mismatch_idx = -1; + int nan_mismatch_count = 0; + bool any_nan = false; - std::size_t num_mismatches = 0; double max_absDiff = 0.0; - std::size_t max_absDiff_ind = 0; + int max_absDiff_idx = -1; + double max_relDiff = 0.0; - std::size_t max_relDiff_ind = 0; - bool has_nan_mismatch = false; - - for (std::size_t i = 0; i < actual.size(); i++) { - double cur_absDiff = std::fabs(actual[i] - desired[i]); - - bool isnan_actual = std::isnan(actual[i]); - bool isnan_desired = std::isnan(desired[i]); - - if ((cur_absDiff > (atol + rtol * std::fabs(desired[i]))) || - (isnan_actual != isnan_desired)) { - num_mismatches++; - if (isnan_actual != isnan_desired) { - has_nan_mismatch = true; - max_absDiff = NAN; - max_absDiff_ind = i; - max_relDiff = NAN; - max_relDiff_ind = i; - } else if (!has_nan_mismatch) { + int max_relDiff_idx = -1; + + auto fn = [&](int flat_idx) { + bool either_nan = + std::isnan(actual[flat_idx]) || std::isnan(desired[flat_idx]); + any_nan = any_nan || either_nan; // <- record whether we have seen a NaN + + // record properties if there is a mismatch + if (!cmp_fn(actual[flat_idx], desired[flat_idx])) { + if (first_mismatch_idx == -1) { + first_mismatch_idx = flat_idx; + } + nan_mismatch_count += either_nan; + if (either_nan && first_nan_mismatch_idx == -1) { + first_nan_mismatch_idx = flat_idx; + } else if (!either_nan) { + double cur_absDiff = std::fabs(actual[flat_idx] - desired[flat_idx]); if (cur_absDiff > max_absDiff) { max_absDiff = cur_absDiff; - max_absDiff_ind = i; + max_absDiff_idx = flat_idx; } - if (cur_absDiff > (max_relDiff * std::fabs(desired[i]))) { - max_relDiff = cur_absDiff / std::fabs(desired[i]); - max_relDiff_ind = i; + if (cur_absDiff > (max_relDiff * std::fabs(desired[flat_idx]))) { + max_relDiff = cur_absDiff / std::fabs(desired[flat_idx]); + max_relDiff_idx = flat_idx; } } } + }; + flat_for_each(fn, idx_mapping.n_elements(), selection_mask); + + // now, let's construct the vector of details + std::vector details; + + if (first_mismatch_idx == -1) { + return details; // <- this is probably indicative of an error + } else { + details.emplace_back("first mismatch", + std::optional{first_mismatch_idx}); + } + + if (max_absDiff_idx == -1) { + details.emplace_back("Max abs diff: NaN (i.e. each mismatch involves NaN)", + std::nullopt); + } else { + details.emplace_back("Max abs diff: " + to_pretty_string(max_absDiff), + std::optional{max_absDiff_idx}); + } + + if (max_relDiff_idx == -1) { + details.emplace_back( + "Max rel diff: NaN (i.e. each mismatch involves NaN or has actual=0.0)", + std::nullopt); + } else { + details.emplace_back("Max rel diff: " + to_pretty_string(max_relDiff), + std::optional{max_relDiff_idx}); + } + + if (first_nan_mismatch_idx == -1) { + details.emplace_back(any_nan ? "all NaNs match" : "there are no NaNs", + std::nullopt); + } else { + details.emplace_back( + "First (of " + std::to_string(nan_mismatch_count) + ") NaN mismatch", + first_nan_mismatch_idx); } - if (num_mismatches == 0) { + return details; +} + +/// Returns a `testing::AssertionResult` instance specifying whether all pairs +/// of values from @p actual and @p desired pointers satisfy the comparison +/// operation specified by @p cmp_fn +/// +/// @tparam T is either `float` or `double` +/// @tparam Layout specifies the data-layout +/// @tparam Cmp Function-like type that does the underlying comparison. See the +/// description of the @p cmp_fn function for more details +/// +/// @param actual,desired The pointers being compared +/// @param idx_mapping Specifies information for treating the pointers as +/// contiguous multi-dimensional arrays. It maps between multi-dimensional +/// indices & pointer 1d offsets, and specifies all relevant information +/// for this mapping (i.e. extents and data layout) +/// @param selection_mask When specified, only the locations holding `true` +/// values are compared +/// @param cmp_fn "Callable" object that implements a function signature +/// equivalent to `bool fun(T actual, T desired)`. This signature is called +/// by passing pairs of values from the @p actual and @p desired pointers. +/// This should implement a member function called `describe_false` that +/// returns a `std::string` +template +testing::AssertionResult compare_ptrs_(const T* actual, const T* desired, + const IdxMapping& idx_mapping, + const bool* selection_mask, Cmp cmp_fn) { + GR_INTERNAL_REQUIRE(actual != nullptr && desired != nullptr, + "it's illegal to compare nullptr"); + // Part 1: perform the comparison (this is as fast as possible) + const int n_elements = idx_mapping.n_elements(); + int mismatch_num = 0; + int n_comparisons = 0; + auto loop_callback = [=, &mismatch_num, &n_comparisons](int flat_idx) { + n_comparisons++; + mismatch_num += !cmp_fn(actual[flat_idx], desired[flat_idx]); + }; + flat_for_each(loop_callback, n_elements, selection_mask); + + if (mismatch_num == 0) { return testing::AssertionSuccess(); } - std::string actual_vec_str = - grtest::ptr_to_string(actual.data(), actual.size()); - std::string ref_vec_str = - grtest::ptr_to_string(desired.data(), desired.size()); - - using grtest::to_pretty_string; - - return testing::AssertionFailure() - << "\narrays are unequal for the tolerance: " - << "rtol = " << to_pretty_string(rtol) << ", " - << "atol = " << to_pretty_string(atol) << '\n' - << "Mismatched elements: " << num_mismatches << " / " << actual.size() - << '\n' - << "Max absolute difference: " << to_pretty_string(max_absDiff) << ", " - << "ind = " << max_absDiff_ind << ", " - << "actual = " << to_pretty_string(actual[max_absDiff_ind]) << ", " - << "reference = " << to_pretty_string(desired[max_absDiff_ind]) << '\n' - << "Max relative difference: " << to_pretty_string(max_relDiff) << ", " - << "ind = " << max_absDiff_ind << ", " - << "actual = " << to_pretty_string(actual[max_relDiff_ind]) << ", " - << "desired = " << to_pretty_string(desired[max_relDiff_ind]) << '\n' - << "actual: " << actual_vec_str << '\n' - << "desired: " << ref_vec_str << '\n'; -} \ No newline at end of file + // Part 2: build the failure result and construct the detailed error message + // -> it's ok if this isn't extremely optimized. This logic shouldn't come up + // very frequently + testing::AssertionResult out = testing::AssertionFailure(); + + out << '\n' + << "arrays are " << cmp_fn.describe_false() << '\n' + << "index mapping: " << testing::PrintToString(idx_mapping) << '\n'; + out << "Mismatched elements: " << mismatch_num << " (" << n_comparisons + << " were compared"; + if (n_comparisons != n_elements) { + out << ", " << n_elements - n_comparisons << "ignored from masking"; + } + out << ")\n"; + + std::vector detail_vec = + collect_details_(actual, desired, idx_mapping, selection_mask, cmp_fn); + if (detail_vec.empty()) { + GR_INTERNAL_ERROR("something went wrong with finding mismatch details"); + } + + // now let's append the interesting mismatch details + for (const MismatchDetail& detail : detail_vec) { + if (!detail.flat_idx.has_value()) { + out << detail.description << '\n'; + continue; + } + int flat_idx = detail.flat_idx.value(); + + out << detail.description << ", "; + // write the index + int idx_components[IdxMapping::MAX_RANK]; + idx_mapping.offset_to_md_idx(flat_idx, idx_components); + out << "idx: {"; + for (int i = 0; i < idx_mapping.rank(); i++) { + out << idx_components[i]; + out << ((i + 1) < idx_mapping.rank() ? ',' : '}'); + } + out << ", "; + + // write the actual and description value + out << "actual = " << to_pretty_string(actual[flat_idx]) << ", " + << "desired = " << to_pretty_string(desired[flat_idx]) << '\n'; + } + + // print out final summary details + bool has_mask = selection_mask != nullptr; + out << "Flattened Ptr Details" + << (has_mask ? " (selection mask is ignored):\n" : ":\n") + << " actual: " << ptr_to_string(actual, idx_mapping) << '\n' + << " desired: " << ptr_to_string(desired, idx_mapping); + return out; +} + +} // anonymous namespace + +testing::AssertionResult compare_(CmpPack pack) { + // this function launches the appropriate specialization of compare_ptrs_ + // -> there are 3 template parameters to consider + // -> (see the docstring in the header file for a little more context) + + // load either (f32_actual, f32_desired) OR (f64_actual, f64_desired) + const float *f32_actual, *f32_desired; + const double *f64_actual, *f64_desired; + bool use_f32 = + std::holds_alternative>(pack.actual_desired_pair); + if (use_f32) { + f32_actual = std::get>(pack.actual_desired_pair).first; + f32_desired = std::get>(pack.actual_desired_pair).second; + } else { + f64_actual = std::get>(pack.actual_desired_pair).first; + f64_desired = std::get>(pack.actual_desired_pair).second; + } + + // Either idx_map_L OR idx_map_R will not be a nullptr + const IdxMapping* idx_map_L = + std::get_if>(&pack.idx_mapping); + const IdxMapping* idx_map_R = + std::get_if>(&pack.idx_mapping); + + // dispatcher_ is a "generic lambda" + // -> it acts as if the type of cmp_fn is a template parameter. + // -> when we pass it to std::visit, the cmp_fn argument is a copy of the + // alternative currently held by the `CmpPack::cmp_fn` variant + auto dispatcher_ = [&](auto cmp_fn) -> testing::AssertionResult { + const bool* smask = pack.selection_mask; + if (use_f32 && idx_map_L != nullptr) { + return compare_ptrs_(f32_actual, f32_desired, *idx_map_L, smask, cmp_fn); + } else if (use_f32 && idx_map_R != nullptr) { + return compare_ptrs_(f32_actual, f32_desired, *idx_map_R, smask, cmp_fn); + } else if (idx_map_L != nullptr) { + return compare_ptrs_(f64_actual, f64_desired, *idx_map_L, smask, cmp_fn); + } else if (idx_map_R != nullptr) { + return compare_ptrs_(f64_actual, f64_desired, *idx_map_R, smask, cmp_fn); + } else { + GR_INTERNAL_ERROR("should be unreachable"); + } + }; + return std::visit(dispatcher_, pack.cmp_fn); +} + +} // namespace grtest::arraycmp_detail \ No newline at end of file diff --git a/tests/grtestutils/googletest/check_allclose.hpp b/tests/grtestutils/googletest/check_allclose.hpp index dedb5049a..615e5e607 100644 --- a/tests/grtestutils/googletest/check_allclose.hpp +++ b/tests/grtestutils/googletest/check_allclose.hpp @@ -12,18 +12,87 @@ #ifndef GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_HPP #define GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_HPP -#include #include + #include +#include "../view.hpp" +#include "./check_allclose_detail.hpp" + +#define COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping) \ + ::grtest::arraycmp_detail::compare_(::grtest::arraycmp_detail::CmpPack{ \ + {cmp_fn}, \ + {::grtest::arraycmp_detail::PtrPair{actual, desired}}, \ + selection_mask, \ + {idx_mapping}}) + +/// Returns whether 2 pointers are exactly equal +/// +/// This draws a lot of inspiration from numpy.testing.assert_array_equal +template +testing::AssertionResult check_array_equal( + const float* actual, const float* desired, + grtest::IdxMapping idx_mapping, + const bool* selection_mask = nullptr) { + grtest::arraycmp_detail::FltIsEqual cmp_fn; + return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); +} + +/// Returns whether 2 pointers are exactly equal +/// +/// This draws a lot of inspiration from numpy.testing.assert_array_equal +template +testing::AssertionResult check_array_equal( + const double* actual, const double* desired, + grtest::IdxMapping idx_mapping, + const bool* selection_mask = nullptr) { + grtest::arraycmp_detail::FltIsEqual cmp_fn; + return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); +} + +/// compares 2 pointers +/// +/// This draws a lot of inspiration from numpy.testing.assert_allclose +template +testing::AssertionResult check_allclose(const float* actual, + const float* desired, + grtest::IdxMapping idx_mapping, + double rtol, double atol = 0.0, + const bool* selection_mask = nullptr) { + grtest::arraycmp_detail::FltIsClose cmp_fn(rtol, atol); + return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); +} + +/// compares 2 pointers +/// +/// This draws a lot of inspiration from numpy.testing.assert_allclose +template +testing::AssertionResult check_allclose(const double* actual, + const double* desired, + grtest::IdxMapping idx_mapping, + double rtol, double atol = 0.0, + const bool* selection_mask = nullptr) { + grtest::arraycmp_detail::FltIsClose cmp_fn(rtol, atol); + return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); +} + +#undef COMPARE_ + /// this compares 2 std::vectors /// /// This draws a lot of inspiration from numpy.testing.assert_allclose /// /// Parts of this are fairly inefficient, partially because it is adapted from /// code written from before we adopted googletest -testing::AssertionResult check_allclose(const std::vector& actual, - const std::vector& desired, - double rtol = 0.0, double atol = 0.0); +inline testing::AssertionResult check_allclose( + const std::vector& actual, const std::vector& desired, + double rtol = 0.0, double atol = 0.0) { + if (actual.size() != desired.size()) { + return testing::AssertionFailure() + << "the compared arrays have different lengths"; + } + grtest::IdxMapping idx_mapping(actual.size()); + return check_allclose(actual.data(), desired.data(), idx_mapping, rtol, atol); +} #endif // GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_HPP diff --git a/tests/grtestutils/googletest/check_allclose_detail.hpp b/tests/grtestutils/googletest/check_allclose_detail.hpp new file mode 100644 index 000000000..9285b5d5c --- /dev/null +++ b/tests/grtestutils/googletest/check_allclose_detail.hpp @@ -0,0 +1,143 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Declares/implements a bunch of helper code to assist with implementing +/// logic in check_allclose.hpp +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_DETAIL_HPP +#define GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_DETAIL_HPP + +#include +#include +#include +#include +#include + +#include "../view.hpp" +#include "gtest/gtest.h" + +namespace grtest::arraycmp_detail { + +template +[[gnu::always_inline]] bool isclose_(T actual, T desired, double rtol, + double atol) { + static_assert(std::is_floating_point_v); + + T abs_diff = std::fabs(actual - desired); + // the following variable is false if actual or desired (or both) is NaN + bool isclose = abs_diff <= atol + rtol * std::fabs(desired); + + if constexpr (EqualNan) { + bool actual_isnan = std::isnan(actual); + bool desired_isnan = std::isnan(desired); + return (actual_isnan && desired_isnan) || isclose; + } else { + return isclose; + } +} + +/// "functor" to check if floating point values are equal within a tolerance +/// +/// This effectively implements numpy's isclose function (see +/// https://numpy.org/doc/stable/reference/generated/numpy.isclose.html). As in +/// the original function, the max allowed variations from the relative +/// difference tolerance and the absolute difference tolerance are summed and +/// compared against the absolute difference. +/// +/// @note +/// For less experienced C++ developers, `operator()` overloads the "function +/// call operation" (it is analogous to python's `__call__` method) +class FltIsClose { + double rtol; + double atol; + +public: + FltIsClose() = delete; + FltIsClose(double rtol, double atol) : rtol{rtol}, atol{atol} {} + + /// determines whether arguments are equal within the tolerance + bool operator()(float actual, float desired) const noexcept { + return isclose_(actual, desired, this->rtol, this->atol); + } + + /// determines whether arguments are equal within the tolerance + bool operator()(double actual, double desired) const noexcept { + return isclose_(actual, desired, this->rtol, this->atol); + } + + /// describe relationship between values for which this functor returns false + std::string describe_false() const { + std::string rtol_str = to_pretty_string(rtol); + std::string atol_str = to_pretty_string(atol); + return ("unequal for the tolerance (rtol = " + rtol_str + + ", atol = " + atol_str + ")"); + } +}; + +/// "functor" to check if floating point values are exactly equal +/// +/// @note +/// For less experienced C++ developers, `operator()` overloads the "function +/// call operation" (it is analogous to python's `__call__` method) +struct FltIsEqual { + /// determines whether arguments are exactly equal + bool operator()(float actual, float desired) const noexcept { + return (actual == desired) || std::isnan(actual) == std::isnan(desired); + } + + /// determines whether arguments are exactly equal + bool operator()(double actual, double desired) const noexcept { + return (actual == desired) || std::isnan(actual) == std::isnan(desired); + } + + /// describe relationship between values for which this functor returns false + std::string describe_false() const { return "not exactly equal"; } +}; + +template +using PtrPair = std::pair; + +/// Packages up the information for a comparison of 2 pointers +/// +/// See the docstring of @ref compare_ for an extended discussion for why +/// this type actually exists. +struct CmpPack { + std::variant cmp_fn; + std::variant, PtrPair> actual_desired_pair; + const bool* selection_mask; + std::variant, IdxMapping> + idx_mapping; +}; + +/// this dispatches the appropriate logic to drive the comparison +/// +/// The most pragmatic approach for implementing the underlying comparisons in +/// an extendable manner (without extensive code duplication or sacrificing +/// performance) is to implement them using templates and to make the datatype +/// a template parameter. +/// +/// This function was designed in a misguided attempt to shift most of the +/// implementation into source files in order to reduce compile times. In order +/// to hide all calls to a set of templates into a source file, this must be +/// a totally ordinary function that dispatches to the proper templates: +/// - thus, @ref CmpPack as a well-defined type to package up all of the +/// possible type combinations. This is achieved through the use of +/// std::variant (i.e. type-safe unions). +/// - the idea is that callers package up `CmpPack`, call this function, and +/// then function unpacks the values from `CmpPack` and dispatches to the +/// appropriate template function. +/// +/// With the benefit of hindsight, this was probably all a mistake... Reducing +/// the compilation cost was probably **NOT** worth the added complexity (if +/// nothing else, we probably should have measured it first...) +testing::AssertionResult compare_(CmpPack pack); + +} // namespace grtest::arraycmp_detail + +#endif // GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_DETAIL_HPP \ No newline at end of file From 8eaefaca8a8f5552a381c0d4cd636c5b71277186 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 24 Feb 2026 09:17:27 -0500 Subject: [PATCH 18/20] lightly refactor test_linalg --- tests/grtestutils/googletest/check_allclose.hpp | 17 ----------------- tests/unit/test_linalg.cpp | 7 +++++-- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/tests/grtestutils/googletest/check_allclose.hpp b/tests/grtestutils/googletest/check_allclose.hpp index 615e5e607..2e34d7977 100644 --- a/tests/grtestutils/googletest/check_allclose.hpp +++ b/tests/grtestutils/googletest/check_allclose.hpp @@ -78,21 +78,4 @@ testing::AssertionResult check_allclose(const double* actual, #undef COMPARE_ -/// this compares 2 std::vectors -/// -/// This draws a lot of inspiration from numpy.testing.assert_allclose -/// -/// Parts of this are fairly inefficient, partially because it is adapted from -/// code written from before we adopted googletest -inline testing::AssertionResult check_allclose( - const std::vector& actual, const std::vector& desired, - double rtol = 0.0, double atol = 0.0) { - if (actual.size() != desired.size()) { - return testing::AssertionFailure() - << "the compared arrays have different lengths"; - } - grtest::IdxMapping idx_mapping(actual.size()); - return check_allclose(actual.data(), desired.data(), idx_mapping, rtol, atol); -} - #endif // GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_HPP diff --git a/tests/unit/test_linalg.cpp b/tests/unit/test_linalg.cpp index d3fb7d080..829349059 100644 --- a/tests/unit/test_linalg.cpp +++ b/tests/unit/test_linalg.cpp @@ -3,7 +3,7 @@ #include "fortran_func_wrappers.hpp" #include "grtestutils/googletest/check_allclose.hpp" - +#include "grtestutils/view.hpp" /// Records the paramters for a linear algebra test-case struct LinAlgCase { @@ -63,7 +63,10 @@ TEST_P(LinAlgTestSolve, CheckSuccessfulSolve) { ASSERT_EQ(rslt, 0) << "expected a return-code of 0, which indicates " << "that the linear equations were successfully solved"; - EXPECT_TRUE(check_allclose(/* actual: */ vec, my_case.solution_vector, + grtest::IdxMapping idx_mapping(vec.size()); + EXPECT_TRUE(check_allclose(/* actual: */ vec.data(), + /* desired: */ my_case.solution_vector.data(), + /* idx_mapping: */ idx_mapping, /* rtol: */1e-15, /* atol: */ 0.0)); } From 9f084926c54cc54578c6f2af450bed106ff67983 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 24 Feb 2026 14:19:41 -0500 Subject: [PATCH 19/20] Address gcc-specific issues --- .../grtestutils/googletest/check_allclose.cpp | 4 ++-- .../grtestutils/googletest/check_allclose.hpp | 19 ++++++++++--------- .../googletest/check_allclose_detail.hpp | 4 ++-- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/tests/grtestutils/googletest/check_allclose.cpp b/tests/grtestutils/googletest/check_allclose.cpp index d13239ffb..b0f9d5602 100644 --- a/tests/grtestutils/googletest/check_allclose.cpp +++ b/tests/grtestutils/googletest/check_allclose.cpp @@ -263,8 +263,8 @@ testing::AssertionResult compare_(CmpPack pack) { // -> (see the docstring in the header file for a little more context) // load either (f32_actual, f32_desired) OR (f64_actual, f64_desired) - const float *f32_actual, *f32_desired; - const double *f64_actual, *f64_desired; + const float *f32_actual = nullptr, *f32_desired = nullptr; + const double *f64_actual = nullptr, *f64_desired = nullptr; bool use_f32 = std::holds_alternative>(pack.actual_desired_pair); if (use_f32) { diff --git a/tests/grtestutils/googletest/check_allclose.hpp b/tests/grtestutils/googletest/check_allclose.hpp index 2e34d7977..6b2686f59 100644 --- a/tests/grtestutils/googletest/check_allclose.hpp +++ b/tests/grtestutils/googletest/check_allclose.hpp @@ -19,12 +19,9 @@ #include "../view.hpp" #include "./check_allclose_detail.hpp" -#define COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping) \ +#define COMPARE_(cmp_fn, ptr_pair, selection_mask, idx_mapping) \ ::grtest::arraycmp_detail::compare_(::grtest::arraycmp_detail::CmpPack{ \ - {cmp_fn}, \ - {::grtest::arraycmp_detail::PtrPair{actual, desired}}, \ - selection_mask, \ - {idx_mapping}}) + {cmp_fn}, {ptr_pair}, selection_mask, {idx_mapping}}) /// Returns whether 2 pointers are exactly equal /// @@ -35,7 +32,8 @@ testing::AssertionResult check_array_equal( grtest::IdxMapping idx_mapping, const bool* selection_mask = nullptr) { grtest::arraycmp_detail::FltIsEqual cmp_fn; - return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); + grtest::arraycmp_detail::PtrPair ptr_pair{actual, desired}; + return COMPARE_(cmp_fn, ptr_pair, selection_mask, idx_mapping); } /// Returns whether 2 pointers are exactly equal @@ -47,7 +45,8 @@ testing::AssertionResult check_array_equal( grtest::IdxMapping idx_mapping, const bool* selection_mask = nullptr) { grtest::arraycmp_detail::FltIsEqual cmp_fn; - return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); + grtest::arraycmp_detail::PtrPair ptr_pair{actual, desired}; + return COMPARE_(cmp_fn, ptr_pair, selection_mask, idx_mapping); } /// compares 2 pointers @@ -60,7 +59,8 @@ testing::AssertionResult check_allclose(const float* actual, double rtol, double atol = 0.0, const bool* selection_mask = nullptr) { grtest::arraycmp_detail::FltIsClose cmp_fn(rtol, atol); - return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); + grtest::arraycmp_detail::PtrPair ptr_pair{actual, desired}; + return COMPARE_(cmp_fn, ptr_pair, selection_mask, idx_mapping); } /// compares 2 pointers @@ -73,7 +73,8 @@ testing::AssertionResult check_allclose(const double* actual, double rtol, double atol = 0.0, const bool* selection_mask = nullptr) { grtest::arraycmp_detail::FltIsClose cmp_fn(rtol, atol); - return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); + grtest::arraycmp_detail::PtrPair ptr_pair{actual, desired}; + return COMPARE_(cmp_fn, ptr_pair, selection_mask, idx_mapping); } #undef COMPARE_ diff --git a/tests/grtestutils/googletest/check_allclose_detail.hpp b/tests/grtestutils/googletest/check_allclose_detail.hpp index 9285b5d5c..a0dd264d6 100644 --- a/tests/grtestutils/googletest/check_allclose_detail.hpp +++ b/tests/grtestutils/googletest/check_allclose_detail.hpp @@ -25,8 +25,8 @@ namespace grtest::arraycmp_detail { template -[[gnu::always_inline]] bool isclose_(T actual, T desired, double rtol, - double atol) { +[[gnu::always_inline]] inline bool isclose_(T actual, T desired, double rtol, + double atol) { static_assert(std::is_floating_point_v); T abs_diff = std::fabs(actual - desired); From e85b00b1aff104766d4b88451e4f195b8bd4552c Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 28 Feb 2026 14:51:40 -0500 Subject: [PATCH 20/20] fixing a minor bug --- .../googletest/check_allclose_detail.hpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/grtestutils/googletest/check_allclose_detail.hpp b/tests/grtestutils/googletest/check_allclose_detail.hpp index a0dd264d6..cdeff25b4 100644 --- a/tests/grtestutils/googletest/check_allclose_detail.hpp +++ b/tests/grtestutils/googletest/check_allclose_detail.hpp @@ -24,6 +24,14 @@ namespace grtest::arraycmp_detail { +/// the goal is to avoid short-circuiting +template +[[gnu::always_inline]] inline bool both_nan_(T actual, T desired) { + bool actual_isnan = std::isnan(actual); + bool desired_isnan = std::isnan(desired); + return actual_isnan & desired_isnan; // use & to avoid &&'s short-circuiting +} + template [[gnu::always_inline]] inline bool isclose_(T actual, T desired, double rtol, double atol) { @@ -34,9 +42,7 @@ template bool isclose = abs_diff <= atol + rtol * std::fabs(desired); if constexpr (EqualNan) { - bool actual_isnan = std::isnan(actual); - bool desired_isnan = std::isnan(desired); - return (actual_isnan && desired_isnan) || isclose; + return both_nan_(actual, desired) || isclose; } else { return isclose; } @@ -88,12 +94,12 @@ class FltIsClose { struct FltIsEqual { /// determines whether arguments are exactly equal bool operator()(float actual, float desired) const noexcept { - return (actual == desired) || std::isnan(actual) == std::isnan(desired); + return (actual == desired) || both_nan_(actual, desired); } /// determines whether arguments are exactly equal bool operator()(double actual, double desired) const noexcept { - return (actual == desired) || std::isnan(actual) == std::isnan(desired); + return (actual == desired) || both_nan_(actual, desired); } /// describe relationship between values for which this functor returns false