From e4df3f914b2dc47af7ab79e7c6530d3d0a2f9e1c Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 19 Dec 2025 08:22:14 -0500 Subject: [PATCH 01/19] address compiler warning in chemistry_solver_funcs.hppp --- src/clib/chemistry_solver_funcs.hpp | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/clib/chemistry_solver_funcs.hpp b/src/clib/chemistry_solver_funcs.hpp index f36db0a7f..ad3f8a760 100644 --- a/src/clib/chemistry_solver_funcs.hpp +++ b/src/clib/chemistry_solver_funcs.hpp @@ -1,6 +1,11 @@ -// See LICENSE file for license and copyright information - -/// @file chemistry_solver_funcs.hpp +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file /// @brief Defines chemistry reaction related functions invoked by the /// grackle solver in order to integrate the species densities over time. /// @@ -14,6 +19,8 @@ /// be decoupled from the derivative calculation for primoridial species /// - it may also make sense to further divide logic by the kinds of species /// that are affected (e.g. primordial vs grains) +/// +//===----------------------------------------------------------------------===// #ifndef CHEMISTRY_SOLVER_FUNCS_HPP #define CHEMISTRY_SOLVER_FUNCS_HPP @@ -579,7 +586,9 @@ inline void species_density_updates_gauss_seidel( if ( (my_chemistry->metal_chemistry == 1) && (itmask_metal[i] != MASK_FALSE) ) { - scoef = scoef; + // we comment out the following line that assigns scoef to itself since + // it has no practical impact and produces a compiler warning + // scoef = scoef; acoef = acoef + kcol_buf.data[CollisionalRxnLUT::kz44][i] * CII(i,j,k) / 12. + kcol_buf.data[CollisionalRxnLUT::kz45][i] * OII(i,j,k) / 16. @@ -1932,7 +1941,9 @@ inline void species_density_derivatives_0d( if ((my_chemistry->metal_chemistry == 1) && (itmask_metal[0] != MASK_FALSE)) { - scoef = scoef; + // we comment out the following line that assigns scoef to itself since + // it has no practical impact and produces a compiler warning + // scoef = scoef; acoef = acoef + kcr_buf.data[CollisionalRxnLUT::kz44][0] * CII / 12. + kcr_buf.data[CollisionalRxnLUT::kz45][0] * OII / 16. From e2ea0d925f6ec099cb9348fbb9675eeb5cc1f263 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 19 Dec 2025 08:23:58 -0500 Subject: [PATCH 02/19] address warning about uninitialized variables in solve_rate_cool_g-cpp.cpp --- src/clib/solve_rate_cool_g-cpp.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clib/solve_rate_cool_g-cpp.cpp b/src/clib/solve_rate_cool_g-cpp.cpp index f96243f58..bde08114e 100644 --- a/src/clib/solve_rate_cool_g-cpp.cpp +++ b/src/clib/solve_rate_cool_g-cpp.cpp @@ -791,7 +791,7 @@ int solve_rate_cool_g( // declare 2 variables (primarily used for subcycling, but also used in // error reporting) int iter; - double ttmin; + double ttmin = huge8; // ------------------ Loop over subcycles ---------------- From 9ff4b3ad7d13286bdb3141560969044dae6ac145 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 11:00:53 -0500 Subject: [PATCH 03/19] mv testing utility library from tests/unit to tests/grtestutils --- .clang-format-ignore | 14 +-- tests/CMakeLists.txt | 1 + tests/grtestutils/CMakeLists.txt | 108 ++++++++++++++++++ tests/grtestutils/README.md | 3 + .../grtest_cmd.cpp => grtestutils/cmd.cpp} | 2 +- .../grtest_cmd.hpp => grtestutils/cmd.hpp} | 0 .../googletest/check_allclose.hpp} | 0 .../grtest_os.cpp => grtestutils/os.cpp} | 2 +- .../grtest_os.hpp => grtestutils/os.hpp} | 0 .../utils.cpp} | 2 +- .../utils.hpp} | 0 tests/unit/CMakeLists.txt | 60 +--------- tests/unit/test_chemistry_struct_synced.cpp | 2 +- tests/unit/test_ghost_zone.cpp | 2 +- tests/unit/test_linalg.cpp | 2 +- tests/unit/test_status_reporting.cpp | 2 +- 16 files changed, 132 insertions(+), 68 deletions(-) create mode 100644 tests/grtestutils/CMakeLists.txt create mode 100644 tests/grtestutils/README.md rename tests/{unit/grtest_cmd.cpp => grtestutils/cmd.cpp} (98%) rename tests/{unit/grtest_cmd.hpp => grtestutils/cmd.hpp} (100%) rename tests/{unit/utest_helpers.hpp => grtestutils/googletest/check_allclose.hpp} (100%) rename tests/{unit/grtest_os.cpp => grtestutils/os.cpp} (99%) rename tests/{unit/grtest_os.hpp => grtestutils/os.hpp} (100%) rename tests/{unit/grtest_utils.cpp => grtestutils/utils.cpp} (98%) rename tests/{unit/grtest_utils.hpp => grtestutils/utils.hpp} (100%) diff --git a/.clang-format-ignore b/.clang-format-ignore index f6fca214f..0072faee9 100644 --- a/.clang-format-ignore +++ b/.clang-format-ignore @@ -64,17 +64,17 @@ src/include/grackle_float.h.in src/include/grackle_misc.h src/include/grackle_rate_functions.h src/include/grackle_types.h -tests/unit/grtest_cmd.cpp -tests/unit/grtest_cmd.hpp -tests/unit/grtest_os.cpp -tests/unit/grtest_os.hpp -tests/unit/grtest_utils.cpp -tests/unit/grtest_utils.hpp +tests/grtestutils/cmd.cpp +tests/grtestutils/cmd.hpp +tests/grtestutils/os.cpp +tests/grtestutils/os.hpp +tests/grtestutils/utils.cpp +tests/grtestutils/utils.hpp +tests/grtestutils/googletest/check_allclose.hpp tests/unit/test_chemistry_struct_synced.cpp tests/unit/test_ghost_zone.cpp tests/unit/test_interpolators_comparisons.cpp tests/unit/test_linalg.cpp tests/unit/test_status_reporting.cpp tests/unit/test_unit_interpolators_g.cpp -tests/unit/utest_helpers.hpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 0af865f10..2320c1c58 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,3 +1,4 @@ +add_subdirectory(grtestutils) add_subdirectory(unit) # down below, we add tests for various code-examples diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt new file mode 100644 index 000000000..cd21e6065 --- /dev/null +++ b/tests/grtestutils/CMakeLists.txt @@ -0,0 +1,108 @@ +# Purpose: Defines 2 "libraries" to aid with the testing of Grackle +# ================================================================= + +# 1. testdeps: bundles dependencies used by all tests +# =================================================== +# - this is an interface library (i.e. nothing is compiled) that ONLY exists +# to aggregate dependencies, include directories and compile definitions +# - in other words, a basic unit test (that doesn't require logic from +# grtestutils can list this as its sole dependency) + +add_library(testdeps INTERFACE) +target_link_libraries(testdeps INTERFACE Grackle::Grackle GTest::gtest_main) + +# compiler defs are short-term hacks to help tests invoke internal functions +# from Grackle (both of these will become unnecessary in the near future) +target_compile_definitions(testdeps + # currently needed to invoke the fortran function wrappers from within clib + INTERFACE "$<$:LINUX>" + # suppresses warnings about C internal headers using our deprecated public + # headers (the files should include grackle.h instead). We're currently + # holding off on properly fixing this to minimize conflicts with pending PRs + # on the newchem-cpp branch + INTERFACE GRIMPL_PUBLIC_INCLUDE=1 +) + +# needed to let tests call internal functions from src/clib +target_include_directories(testdeps INTERFACE ${PROJECT_SOURCE_DIR}/src/clib) +target_compile_features(testdeps INTERFACE cxx_std_17) + + +# 2. grtestutils: testing utility library +# ======================================= +# - this is an internal library that defines reusable testing utilities +# - Frankly, I don't love the name. Originally I called it grtest, but I'm a +# a little concerned that may look a little too much like gtest (i.e. the +# shorthand that googletest uses. + +# Code Organization +# ----------------- +# This code in this directory is organized in a slightly peculiar manner +# - code in the main directory should not depend upon googletest +# - the googletest subdirectory is intended to define custom googletest +# assertions and fixtures. This subdirectory should **ONLY** include +# headers (so that, at most, this internal library has a transitive +# dependence on googletest) +# +# RATIONALE: +# - over time, the idea has been floated to introducing C++ logic to +# accelerate certain calculations provided by the python wrapper: +# - PR #343 proposed creating a C++ accelerated harness for performing +# certain kinds of semi-analytic calculations (like freefall or evolving +# constant density) +# - PR #370 proposed introducing a function to infer the internal energy at +# a desired temperature (yes, the PR proposes it as a python function, but +# it would be faster as a C++ function). +# - Relatedly, it would also be useful to be able to directly compute +# temperature or chemical equilibrium +# - in each case this functionality would also be extremely useful for the +# googletest framework or for writing simple C++-only benchmarking programs +# - this is extremely useful if you want to use tools like valgrind or +# compiler sanitizers. While it's generally possible to use these tools in +# a python extension module, it may require compiling CPython itself from +# source. Moreover it can be extremely slow (these tools introduce to +# basic C/C++ operations and that overhead will be apply to the Python +# interpretter) +# - this is important if we want to test grackle when compiled with various +# backends. Historically, the python bindings have never been able to use +# Grackle with the OpenMP backend. While we can and will address this, +# there will additional challenges as we introduce GPU acceleration +# - this library is intended as a location where that kind of hypothetical +# functionality can be introduced, and organization strategy ensures that +# it will be straightforward to provide this hypothetical functionality to +# the python bindings without it depending upon googletest (i.e. we may +# treat the googletest subdirectory as a distinct interface library) + +add_library(grtestutils + cmd.hpp cmd.cpp + utils.hpp utils.cpp + os.hpp os.cpp + + # REMINDER: the googletest subdirectory should only contain headers + googletest/check_allclose.hpp +) + +# we are being a little lazy with our usage of testdeps right here +target_link_libraries(grtestutils PUBLIC testdeps) +target_compile_features(grtestutils PUBLIC cxx_std_17) + +# configure the grtestutils target so that when a file outside of this +# directory includes a header from within this directory, the include +# directive looks something like: +# #include "grtestutils/.hpp" +# OR +# #include "grtestutils/googletest/.hpp +target_include_directories(grtestutils INTERFACE ${PROJECT_SOURCE_DIR}/tests) + +# these compile-definitions act as short-term hacks +target_compile_definitions(grtestutils + # this hack helps us get the path to the directory holding data files (we can + # remove it once we introduce automatic file management in PR 235, PR 237, + # and PR 246) + PRIVATE GR_DATADIR=${CMAKE_CURRENT_SOURCE_DIR}/../../grackle_data_files/input/ + + # this hack lets us use Operating-system specific functionality (Once PR #237 + # is merged, we should make use of the machinery introduced by that PR for + # enabling/disabling os-specific features) + PRIVATE "$<$:PLATFORM_GENERIC_UNIX>" +) diff --git a/tests/grtestutils/README.md b/tests/grtestutils/README.md new file mode 100644 index 000000000..d7d59171f --- /dev/null +++ b/tests/grtestutils/README.md @@ -0,0 +1,3 @@ +Defines a simple testing utility library. + +See the CMakeLists.txt for a discussion of code organization. diff --git a/tests/unit/grtest_cmd.cpp b/tests/grtestutils/cmd.cpp similarity index 98% rename from tests/unit/grtest_cmd.cpp rename to tests/grtestutils/cmd.cpp index 131fa158b..700cafa01 100644 --- a/tests/unit/grtest_cmd.cpp +++ b/tests/grtestutils/cmd.cpp @@ -1,4 +1,4 @@ -#include "grtest_cmd.hpp" +#include "cmd.hpp" #include // FILE, fgets, fprintf, stderr, (popen/pclose on POSIX) diff --git a/tests/unit/grtest_cmd.hpp b/tests/grtestutils/cmd.hpp similarity index 100% rename from tests/unit/grtest_cmd.hpp rename to tests/grtestutils/cmd.hpp diff --git a/tests/unit/utest_helpers.hpp b/tests/grtestutils/googletest/check_allclose.hpp similarity index 100% rename from tests/unit/utest_helpers.hpp rename to tests/grtestutils/googletest/check_allclose.hpp diff --git a/tests/unit/grtest_os.cpp b/tests/grtestutils/os.cpp similarity index 99% rename from tests/unit/grtest_os.cpp rename to tests/grtestutils/os.cpp index eba65b404..bbcd4538e 100644 --- a/tests/unit/grtest_os.cpp +++ b/tests/grtestutils/os.cpp @@ -1,4 +1,4 @@ -#include "grtest_os.hpp" +#include "os.hpp" // the following 2 headers are the c versions of the headers (rather than the // C++ versions) since it seems more likely that posix-specific functions are diff --git a/tests/unit/grtest_os.hpp b/tests/grtestutils/os.hpp similarity index 100% rename from tests/unit/grtest_os.hpp rename to tests/grtestutils/os.hpp diff --git a/tests/unit/grtest_utils.cpp b/tests/grtestutils/utils.cpp similarity index 98% rename from tests/unit/grtest_utils.cpp rename to tests/grtestutils/utils.cpp index 85557d7af..d4781249f 100644 --- a/tests/unit/grtest_utils.cpp +++ b/tests/grtestutils/utils.cpp @@ -1,4 +1,4 @@ -#include "grtest_utils.hpp" +#include "utils.hpp" #include diff --git a/tests/unit/grtest_utils.hpp b/tests/grtestutils/utils.hpp similarity index 100% rename from tests/unit/grtest_utils.hpp rename to tests/grtestutils/utils.hpp diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index f1018e3ca..6bac6b918 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -1,60 +1,12 @@ -# declare testdeps to bundle together dependencies used by all tests -# ------------------------------------------------------------------ -add_library(testdeps INTERFACE) -target_link_libraries(testdeps INTERFACE Grackle::Grackle GTest::gtest_main) +# NOTE: the grtestutils and testdeps library targets are both defined within +# ../grtestutils/CMakeLists.txt -# compiler defs are short-term hacks to help tests invoke internal functions -# from Grackle (both of these will become unnecessary in the near future) -target_compile_definitions(testdeps - # needed to invoke Fortran functions from C - INTERFACE "$<$:LINUX>" - # suppresses warnings about C internal headers using our deprecated public - # headers (the files should include grackle.h instead). We're currently - # holding off on properly fixing this to minimize conflicts with pending PRs - # on the newchem-cpp branch - INTERFACE GRIMPL_PUBLIC_INCLUDE=1 -) - -# needed to let tests call internal functions from C -target_include_directories(testdeps INTERFACE ${PROJECT_SOURCE_DIR}/src/clib) -target_compile_features(testdeps INTERFACE cxx_std_17) - -# declare the grtest utility library -# ---------------------------------- -# -> this is an internal library that defines reusable testing utilities -# -> if we add more files to this library, we should consider relocating the -# library to a different directory - -add_library(grtest_utils - grtest_cmd.hpp grtest_cmd.cpp - grtest_utils.hpp grtest_utils.cpp - grtest_os.hpp grtest_os.cpp -) -# we are being a little lazy with our usage of testdeps right here -target_link_libraries(grtest_utils PUBLIC testdeps) -target_compile_features(grtest_utils PUBLIC cxx_std_17) - -# these compile-definitions act as short-term hacks -target_compile_definitions(grtest_utils - # this hack helps us get path input-file directory (we can remove it once we - # introduce automatic file management in PR 235, PR 237, and PR 246) - PRIVATE GR_DATADIR=${CMAKE_CURRENT_SOURCE_DIR}/../../grackle_data_files/input/ - - # this hack lets us use Operating-system specific functionality (Once PR #237 - # is merged, we should make use of the machinery introduced by that PR for - # enabling/disabling os-specific features) - PRIVATE "$<$:PLATFORM_GENERIC_UNIX>" -) - -# start declaring targets for tests -# --------------------------------- add_executable(runInterpolationTests test_unit_interpolators_g.cpp) target_link_libraries(runInterpolationTests testdeps) - gtest_discover_tests(runInterpolationTests) add_executable(runLinAlgTests test_linalg.cpp) -target_link_libraries(runLinAlgTests testdeps) +target_link_libraries(runLinAlgTests grtestutils) gtest_discover_tests(runLinAlgTests) add_executable(runGrainSpeciesIntoTests test_grain_species_info.cpp) @@ -62,7 +14,7 @@ target_link_libraries(runGrainSpeciesIntoTests testdeps) gtest_discover_tests(runGrainSpeciesIntoTests) add_executable(runStatusReporting test_status_reporting.cpp) -target_link_libraries(runStatusReporting grtest_utils) +target_link_libraries(runStatusReporting grtestutils) gtest_discover_tests(runStatusReporting) # one might argue that the following is more of an integration or end-to-end @@ -74,7 +26,7 @@ gtest_discover_tests(runVisitorTests) # one might argue that the following is more of an integration or end-to-end # test than a unit-test add_executable(runGhostZoneTests test_ghost_zone.cpp) -target_link_libraries(runGhostZoneTests grtest_utils testdeps) +target_link_libraries(runGhostZoneTests grtestutils testdeps) gtest_discover_tests(runGhostZoneTests) # this target tests that the members of the chemistry_data struct can be @@ -84,7 +36,7 @@ gtest_discover_tests(runGhostZoneTests) # problems for "death-tests", so these test-cases should remain separate from # the rest of the gtest framework add_executable(runSyncedChemistryData test_chemistry_struct_synced.cpp) -target_link_libraries(runSyncedChemistryData grtest_utils testdeps) +target_link_libraries(runSyncedChemistryData grtestutils testdeps) target_compile_definitions(runSyncedChemistryData PRIVATE READER_PATH=${PROJECT_SOURCE_DIR}/tests/scripts/castxml_output_reader.py diff --git a/tests/unit/test_chemistry_struct_synced.cpp b/tests/unit/test_chemistry_struct_synced.cpp index 6a3bd9642..ee02a02bb 100644 --- a/tests/unit/test_chemistry_struct_synced.cpp +++ b/tests/unit/test_chemistry_struct_synced.cpp @@ -1,4 +1,4 @@ -#include "grtest_cmd.hpp" +#include "grtestutils/cmd.hpp" #include diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index b009fbe7b..607da2325 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -24,7 +24,7 @@ #include #include -#include "grtest_utils.hpp" +#include "grtestutils/utils.hpp" #include diff --git a/tests/unit/test_linalg.cpp b/tests/unit/test_linalg.cpp index 943b83d75..d3fb7d080 100644 --- a/tests/unit/test_linalg.cpp +++ b/tests/unit/test_linalg.cpp @@ -2,7 +2,7 @@ #include #include "fortran_func_wrappers.hpp" -#include "utest_helpers.hpp" +#include "grtestutils/googletest/check_allclose.hpp" /// Records the paramters for a linear algebra test-case diff --git a/tests/unit/test_status_reporting.cpp b/tests/unit/test_status_reporting.cpp index 1922fef2a..91ce3791c 100644 --- a/tests/unit/test_status_reporting.cpp +++ b/tests/unit/test_status_reporting.cpp @@ -12,7 +12,7 @@ #include "grackle.h" // GR_FAIL #include "status_reporting.h" -#include "grtest_os.hpp" +#include "grtestutils/os.hpp" testing::AssertionResult ContainsFormattedMessage_(int n) { From b5b7e65e1461923b944031514e2efa3b299f4536 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 12:47:21 -0500 Subject: [PATCH 04/19] slightly alter we define targets that support testing --- tests/grtestutils/CMakeLists.txt | 83 +++++++++++++++++++++----------- tests/unit/CMakeLists.txt | 11 ++--- 2 files changed, 61 insertions(+), 33 deletions(-) diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index cd21e6065..42675d148 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -1,20 +1,16 @@ -# Purpose: Defines 2 "libraries" to aid with the testing of Grackle -# ================================================================= +# Purpose: define "libraries" to aid with the testing of Grackle +# ============================================================== -# 1. testdeps: bundles dependencies used by all tests -# =================================================== -# - this is an interface library (i.e. nothing is compiled) that ONLY exists -# to aggregate dependencies, include directories and compile definitions -# - in other words, a basic unit test (that doesn't require logic from -# grtestutils can list this as its sole dependency) +# first, define an interface "library" (i.e. nothing is compiled) for +# book-keeping purposes. dependents get "linked" against this library in order +# to be able to access grackle's internal functionality -add_library(testdeps INTERFACE) -target_link_libraries(testdeps INTERFACE Grackle::Grackle GTest::gtest_main) +add_library(_grackle_internals INTERFACE) +target_link_libraries(_grackle_internals INTERFACE Grackle::Grackle) -# compiler defs are short-term hacks to help tests invoke internal functions -# from Grackle (both of these will become unnecessary in the near future) -target_compile_definitions(testdeps - # currently needed to invoke the fortran function wrappers from within clib +# short-term macro definitions (both will become unnecessary in the near future) +target_compile_definitions(_grackle_internals + # currently required for functions declared by fortran_func_wrappers.hpp INTERFACE "$<$:LINUX>" # suppresses warnings about C internal headers using our deprecated public # headers (the files should include grackle.h instead). We're currently @@ -23,17 +19,24 @@ target_compile_definitions(testdeps INTERFACE GRIMPL_PUBLIC_INCLUDE=1 ) -# needed to let tests call internal functions from src/clib -target_include_directories(testdeps INTERFACE ${PROJECT_SOURCE_DIR}/src/clib) -target_compile_features(testdeps INTERFACE cxx_std_17) - +# this makes it possible to write an include-directive of a header from +# src/clib, by writing something like +# #include "{name}.hpp" +# where {name} is replaced by the name of the header +# -> frankly, I don't love this because it's not immediately clear that we are +# including a private header. The alternative is to require the paths +# in the include directives to include the name of the directory holding +# the implementation files (e.g. "clib/{header}.hpp") +target_include_directories(_grackle_internals + INTERFACE ${PROJECT_SOURCE_DIR}/src/clib +) -# 2. grtestutils: testing utility library -# ======================================= +# grtestutils: testing utility library +# ==================================== # - this is an internal library that defines reusable testing utilities # - Frankly, I don't love the name. Originally I called it grtest, but I'm a # a little concerned that may look a little too much like gtest (i.e. the -# shorthand that googletest uses. +# shorthand that googletest uses). # Code Organization # ----------------- @@ -68,22 +71,32 @@ target_compile_features(testdeps INTERFACE cxx_std_17) # Grackle with the OpenMP backend. While we can and will address this, # there will additional challenges as we introduce GPU acceleration # - this library is intended as a location where that kind of hypothetical -# functionality can be introduced, and organization strategy ensures that +# functionality can be introduced. The organization strategy ensures that # it will be straightforward to provide this hypothetical functionality to -# the python bindings without it depending upon googletest (i.e. we may -# treat the googletest subdirectory as a distinct interface library) +# the python bindings (and any C++ testing code) without making the python +# bindings link against googletest. add_library(grtestutils + # files outside of the googletest subdirectory + # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp utils.hpp utils.cpp os.hpp os.cpp - # REMINDER: the googletest subdirectory should only contain headers + # files in the googletest subdirectory (reminder: should just be headers!) googletest/check_allclose.hpp ) -# we are being a little lazy with our usage of testdeps right here -target_link_libraries(grtestutils PUBLIC testdeps) +target_link_libraries(grtestutils + PRIVATE _grackle_internals + PUBLIC Grackle::Grackle + # to express the transitive dependency on googletest (it's transitive since + # its only referenced by header files), we should theoretically write + # INTERFACE GTest::GTest + # we choose not to do this since all test code pulls in this dependency by + # explicitly listing `testdeps` as a dependency (defined below) +) + target_compile_features(grtestutils PUBLIC cxx_std_17) # configure the grtestutils target so that when a file outside of this @@ -106,3 +119,19 @@ target_compile_definitions(grtestutils # enabling/disabling os-specific features) PRIVATE "$<$:PLATFORM_GENERIC_UNIX>" ) + + +# 2. testdeps: bundles dependencies used by tests +# =============================================== +# - this is an interface library (i.e. nothing is compiled) that ONLY exists +# to aggregate dependencies, include directories and compile definitions +# - all tests should depend on this target + +add_library(testdeps INTERFACE) +target_link_libraries(testdeps + INTERFACE Grackle::Grackle grtestutils _grackle_internals GTest::gtest_main +) +# indicates that all consumers are written in C++17 or newer +target_compile_features(testdeps INTERFACE cxx_std_17) + + diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 6bac6b918..fd5a75ac1 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -1,12 +1,11 @@ -# NOTE: the grtestutils and testdeps library targets are both defined within -# ../grtestutils/CMakeLists.txt +# NOTE: the testdeps target is defined within ../grtestutils/CMakeLists.txt add_executable(runInterpolationTests test_unit_interpolators_g.cpp) target_link_libraries(runInterpolationTests testdeps) gtest_discover_tests(runInterpolationTests) add_executable(runLinAlgTests test_linalg.cpp) -target_link_libraries(runLinAlgTests grtestutils) +target_link_libraries(runLinAlgTests testdeps) gtest_discover_tests(runLinAlgTests) add_executable(runGrainSpeciesIntoTests test_grain_species_info.cpp) @@ -14,7 +13,7 @@ target_link_libraries(runGrainSpeciesIntoTests testdeps) gtest_discover_tests(runGrainSpeciesIntoTests) add_executable(runStatusReporting test_status_reporting.cpp) -target_link_libraries(runStatusReporting grtestutils) +target_link_libraries(runStatusReporting testdeps) gtest_discover_tests(runStatusReporting) # one might argue that the following is more of an integration or end-to-end @@ -26,7 +25,7 @@ gtest_discover_tests(runVisitorTests) # one might argue that the following is more of an integration or end-to-end # test than a unit-test add_executable(runGhostZoneTests test_ghost_zone.cpp) -target_link_libraries(runGhostZoneTests grtestutils testdeps) +target_link_libraries(runGhostZoneTests testdeps) gtest_discover_tests(runGhostZoneTests) # this target tests that the members of the chemistry_data struct can be @@ -36,7 +35,7 @@ gtest_discover_tests(runGhostZoneTests) # problems for "death-tests", so these test-cases should remain separate from # the rest of the gtest framework add_executable(runSyncedChemistryData test_chemistry_struct_synced.cpp) -target_link_libraries(runSyncedChemistryData grtestutils testdeps) +target_link_libraries(runSyncedChemistryData testdeps) target_compile_definitions(runSyncedChemistryData PRIVATE READER_PATH=${PROJECT_SOURCE_DIR}/tests/scripts/castxml_output_reader.py From 8cae77700db4cda8599c9563237252ddf1fd588f Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 14:37:13 -0500 Subject: [PATCH 05/19] address a todo item (to reduce code duplication) --- tests/grtestutils/cmd.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/tests/grtestutils/cmd.cpp b/tests/grtestutils/cmd.cpp index 700cafa01..36128da0b 100644 --- a/tests/grtestutils/cmd.cpp +++ b/tests/grtestutils/cmd.cpp @@ -1,18 +1,13 @@ #include "cmd.hpp" +// internal Grackle routine: +#include "status_reporting.h" // GR_INTERNAL_ERROR, GR_INTERNAL_REQUIRE + #include // FILE, fgets, fprintf, stderr, (popen/pclose on POSIX) -#include // std::exit #include // std::stringstream #include -// TODO: replace with Grackle's existing err machinery -[[noreturn]] static void err_(std::string msg="") { - const char* ptr = (msg.empty()) ? "" : msg.c_str(); - fprintf(stderr, "ERROR: %s\n", ptr); - std::exit(1); -} - #ifdef PLATFORM_GENERIC_UNIX #define TEMP_BUF_SIZE 128 @@ -29,14 +24,14 @@ grtest::ProcessStatusAndStdout grtest::capture_status_and_output( // fp represents a buffered pipe to the standard output of the command. FILE* fp = popen(command.data(), "r"); - if (fp == nullptr) { err_("there was a problem launching the command"); } + GR_INTERNAL_REQUIRE(fp != nullptr, "there's a problem launching the command"); // if our reads from the pipe outpace the rate at which the command writes to // the pipe, fgets won't return until more data becomes available. If the // processess ends, fgets encounters EOF (causing fgets to return) while (fgets(temp_buf, TEMP_BUF_SIZE, fp) != nullptr) { buf << temp_buf; } int status = pclose(fp); - if (status == -1) { err_("there was a problem closing the command"); } + GR_INTERNAL_REQUIRE(status != -1, "there was a problem closing the command"); return {status, buf.str()}; } @@ -45,7 +40,7 @@ grtest::ProcessStatusAndStdout grtest::capture_status_and_output( grtest::ProcessStatusAndStdout grtest::capture_status_and_output( const std::string& command ) { - err_("not implemented on this platform"); + GR_INTERNAL_ERROR("not implemented on this platform"); } #endif /* PLATFORM_GENERIC_UNIX */ From 5cf43bca95b2e26b28d640f692b65098cd91ec41 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 19 Dec 2025 11:11:07 -0500 Subject: [PATCH 06/19] Add gmock to the list of libraries linked via testdeps For context, the gmock library is a component of googletest (it's already being installed). All this commit does is make it possible for us to use it in subsequent PRs --- tests/grtestutils/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index 42675d148..e58b7322b 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -129,7 +129,8 @@ target_compile_definitions(grtestutils add_library(testdeps INTERFACE) target_link_libraries(testdeps - INTERFACE Grackle::Grackle grtestutils _grackle_internals GTest::gtest_main + INTERFACE Grackle::Grackle grtestutils _grackle_internals + GTest::gtest_main GTest::gmock ) # indicates that all consumers are written in C++17 or newer target_compile_features(testdeps INTERFACE cxx_std_17) From deb6c84a93c7537a3c4fb9a39beb19dc96a996b6 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 16 Dec 2025 16:33:32 -0500 Subject: [PATCH 07/19] mv GrackleCtxPack out of test_ghost_zone and refactor --- src/clib/status_reporting.h | 16 +++ tests/grtestutils/CMakeLists.txt | 4 +- tests/grtestutils/googletest/fixtures.hpp | 110 +++++++++++++++ tests/grtestutils/preset.cpp | 107 +++++++++++++++ tests/grtestutils/preset.hpp | 66 +++++++++ tests/unit/test_ghost_zone.cpp | 157 +++++++--------------- 6 files changed, 351 insertions(+), 109 deletions(-) create mode 100644 tests/grtestutils/googletest/fixtures.hpp create mode 100644 tests/grtestutils/preset.cpp create mode 100644 tests/grtestutils/preset.hpp diff --git a/src/clib/status_reporting.h b/src/clib/status_reporting.h index f52f24651..fe1a1cd5b 100644 --- a/src/clib/status_reporting.h +++ b/src/clib/status_reporting.h @@ -320,6 +320,22 @@ ERRFMT_ATTR_(2) void grimpl_print_err_msg_( #define GrPrintErrMsg(...) \ grimpl_print_err_msg_(__GRIMPL_SRCLOC__, __VA_ARGS__); +/// @def GR_INTERNAL_UNREACHABLE_ERROR() +/// @brief function-like macro that aborts with a (lethal) error message +/// indicating that +/// +/// This macro should be treated as a function with the signature: +/// +/// [[noreturn]] void GR_INTERNAL_UNREACHABLE_ERROR(); +/// +/// @note +/// Unlike gcc/clang's __builtin_unreachable or C++23's std::unreachable, this +/// aborts the program with an error if its executed (the other cases produce +/// undefined behavior). (An argument could be made for conditionally compiling +/// this macro into the alternatives to test speed) +#define GR_INTERNAL_UNREACHABLE_ERROR() \ +{ grimpl_abort_with_internal_err_(__GRIMPL_SRCLOC__, \ +"location shouldn't be reachable"); } // undefine the attributes so we avoid leaking them // ------------------------------------------------ diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index e58b7322b..f05d4ff1f 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -80,11 +80,13 @@ add_library(grtestutils # files outside of the googletest subdirectory # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp - utils.hpp utils.cpp os.hpp os.cpp + preset.hpp preset.cpp + utils.hpp utils.cpp # files in the googletest subdirectory (reminder: should just be headers!) googletest/check_allclose.hpp + googletest/fixtures.hpp ) target_link_libraries(grtestutils diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp new file mode 100644 index 000000000..b1bc74969 --- /dev/null +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -0,0 +1,110 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Define machinery for creating GoogleTest Fixutures to help test Grackle's +/// C API. +/// +//===----------------------------------------------------------------------===// + +#ifndef GRTEST_FIXTURE +#define GRTEST_FIXTURE + +#include +// 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 + +#include "grackle.h" +#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 // GRTEST_FIXTURE diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp new file mode 100644 index 000000000..22621cc18 --- /dev/null +++ b/tests/grtestutils/preset.cpp @@ -0,0 +1,107 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// implement logic pertaining to pre-defined configuration presets +/// +//===----------------------------------------------------------------------===// + +#include "./preset.hpp" +#include "./utils.hpp" + +#include "grackle.h" +#include "status_reporting.h" // GR_INTERNAL_UNREACHABLE_ERROR + +static std::string to_string(const grtest::ChemPreset& preset) { + switch (preset) { + case grtest::ChemPreset::primchem0: + return "pc=0"; + case grtest::ChemPreset::primchem1: + return "pc=1"; + case grtest::ChemPreset::primchem2: + return "pc=2"; + case grtest::ChemPreset::primchem3: + return "pc=3"; + } + + GR_INTERNAL_UNREACHABLE_ERROR(); +} + +grtest::InitStatus grtest::setup_chemistry_data_from_preset( + chemistry_data* my_chem, ChemPreset preset) { + if (local_initialize_chemistry_parameters(my_chem) != GR_SUCCESS) { + return InitStatus::generic_fail; + } + + if (!grtest::set_standard_datafile(*my_chem, "CloudyData_UVB=HM2012.h5")) { + return InitStatus::datafile_notfound; + } + + 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; + } + case ChemPreset::primchem1: { + my_chem->primordial_chemistry = 1; + my_chem->dust_chemistry = 1; + return InitStatus::success; + } + case ChemPreset::primchem2: { + my_chem->primordial_chemistry = 2; + my_chem->dust_chemistry = 1; + return InitStatus::success; + } + case ChemPreset::primchem3: { + my_chem->primordial_chemistry = 3; + my_chem->dust_chemistry = 1; + return InitStatus::success; + } + } + GR_INTERNAL_UNREACHABLE_ERROR(); +} + +static std::string to_string(const grtest::InitialUnitPreset& preset) { + switch (preset) { + case grtest::InitialUnitPreset::simple_z0: + return "simpleUnit-z=0"; + } + GR_INTERNAL_UNREACHABLE_ERROR(); +} + +code_units grtest::setup_initial_unit(grtest::InitialUnitPreset preset) { + // since we return in the switch statement, the compiler should always warn + // us if we're missing an enumeration + switch (preset) { + case InitialUnitPreset::simple_z0: { + double initial_redshift = 0.; + code_units my_units; + my_units.comoving_coordinates = 0; // 1 if cosmological sim, 0 if not + my_units.density_units = 1.67e-24; + my_units.length_units = 1.0; + my_units.time_units = 1.0e12; + my_units.a_units = 1.0; // units for the expansion factor + // Set expansion factor to 1 for non-cosmological simulation. + my_units.a_value = 1. / (1. + initial_redshift) / my_units.a_units; + set_velocity_units(&my_units); + return my_units; + } + } + GR_INTERNAL_UNREACHABLE_ERROR(); +} + +void grtest::PrintTo(const grtest::FullConfPreset& 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 new file mode 100644 index 000000000..5f62c4e7b --- /dev/null +++ b/tests/grtestutils/preset.hpp @@ -0,0 +1,66 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// declare some standard pre-defined configuration presets +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_PRESET_HPP +#define GRTESTUTILS_PRESET_HPP + +#include "grackle.h" +#include +#include + +namespace grtest { + +/// this only exists so that we can determine the reason that a test fails +enum class InitStatus { + success, + generic_fail, + datafile_notfound, +}; + +/// represents different presets for initializing chemistry_data +/// +/// @note +/// In the future, we probably want to add more +enum class ChemPreset { + primchem0, + primchem1, + primchem2, + primchem3, +}; + +/// override the settings of my_chem based on the specified preset +InitStatus setup_chemistry_data_from_preset(chemistry_data* my_chem, + ChemPreset preset); + +/// Preset for constructing the code_unit instance used for initializing the +/// Grackle Solver +/// +/// @note +/// In the future, we probably want to add more +enum class InitialUnitPreset { + simple_z0, // <- no cosmology, z=0 +}; + +/// 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 FullConfPreset { + ChemPreset chemistry; + InitialUnitPreset unit; +}; + +// teach googletest how to print FullConfPreset +void PrintTo(const FullConfPreset& preset, std::ostream* os); + +} // namespace grtest + +#endif // GRTESTUTILS_PRESET_HPP diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index 607da2325..2ee3807e5 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -25,6 +25,7 @@ #include #include "grtestutils/utils.hpp" +#include "grtestutils/googletest/fixtures.hpp" #include @@ -33,7 +34,8 @@ #define mh 1.67262171e-24 #define kboltz 1.3806504e-16 -typedef int (*property_func)(code_units*, grackle_field_data*, gr_float*); +typedef int (*property_func)(chemistry_data*, chemistry_data_storage*, + code_units*, grackle_field_data*, gr_float*); typedef std::map> val_vec_map_t; @@ -79,7 +81,8 @@ class FieldInitHelper{ // allocates the grackle_field_data struct void construct_field_data(grackle_field_data& my_fields, grid_props& my_grid_props, - code_units& my_units, + const chemistry_data* my_chemistry, + const code_units& my_units, val_vec_map_t& val_map, std::minstd_rand& generator){ @@ -162,11 +165,11 @@ void construct_field_data(grackle_field_data& my_fields, for (int ix = gx; ix < (mx - gx); ix++){ int i = ix + mx * (iy + my*iz); my_fields.density[i] = 1.0; - my_fields.HI_density[i] = grackle_data->HydrogenFractionByMass * + my_fields.HI_density[i] = my_chemistry->HydrogenFractionByMass * my_fields.density[i]; my_fields.HII_density[i] = tiny_number * my_fields.density[i]; my_fields.HM_density[i] = tiny_number * my_fields.density[i]; - my_fields.HeI_density[i] = (1.0 - grackle_data->HydrogenFractionByMass) + my_fields.HeI_density[i] = (1.0 - my_chemistry->HydrogenFractionByMass) * my_fields.density[i]; my_fields.HeII_density[i] = tiny_number * my_fields.density[i]; my_fields.HeIII_density[i] = tiny_number * my_fields.density[i]; @@ -177,7 +180,7 @@ void construct_field_data(grackle_field_data& my_fields, my_fields.HDI_density[i] = tiny_number * my_fields.density[i]; my_fields.e_density[i] = tiny_number * my_fields.density[i]; // solar metallicity - my_fields.metal_density[i] = grackle_data->SolarMetalFractionByMass * + my_fields.metal_density[i] = my_chemistry->SolarMetalFractionByMass * my_fields.density[i]; my_fields.x_velocity[i] = 0.0; @@ -196,7 +199,7 @@ void construct_field_data(grackle_field_data& my_fields, my_fields.RT_H2_dissociation_rate[i] = 0.0; my_fields.RT_heating_rate[i] = 0.0; - my_fields.isrf_habing[i] = grackle_data->interstellar_radiation_field; + my_fields.isrf_habing[i] = my_chemistry->interstellar_radiation_field; } } } @@ -246,77 +249,8 @@ bool equal_ghost_values(val_vec_map_t& ref, val_vec_map_t& actual, return true; } -namespace { // stuff within anonymous namespace is local to the current file - -/// the following is just a dummy struct that primarily exists to assist with -/// cleanup (and avoid memory leaks) -struct GrackleCtxPack { - bool successful_default = false; - bool successful_data_file = false; - bool successful_init = false; - code_units my_units; - chemistry_data* my_chemistry = nullptr; -}; - -void cleanup_grackle_conditions(GrackleCtxPack& pack) { - if (pack.successful_init) { free_chemistry_data(); } - if (pack.my_chemistry != nullptr) { delete pack.my_chemistry; } -} - -GrackleCtxPack setup_simple_grackle_conditions(int primordial_chemistry) { - /********************************************************************* - / Initial setup of units and chemistry objects. - *********************************************************************/ - - GrackleCtxPack pack; - - // Set initial redshift (for internal units). - double initial_redshift = 0.; - - // First, set up the units system. - // These are conversions from code units to cgs. - pack.my_units.comoving_coordinates = 0; // 1 if cosmological sim, 0 if not - pack.my_units.density_units = 1.67e-24; - pack.my_units.length_units = 1.0; - pack.my_units.time_units = 1.0e12; - pack.my_units.a_units = 1.0; // units for the expansion factor - // Set expansion factor to 1 for non-cosmological simulation. - pack.my_units.a_value = 1. / (1. + initial_redshift) / pack.my_units.a_units; - set_velocity_units(&pack.my_units); - - // Second, create a chemistry object for parameters. - pack.my_chemistry = new chemistry_data; - if (set_default_chemistry_parameters(pack.my_chemistry) != GR_SUCCESS) { - return pack; - } - pack.successful_default=true; - - // Set parameter values for chemistry. - // Access the parameter storage with the struct you've created - // or with the grackle_data pointer declared in grackle.h (see further below). - pack.my_chemistry->use_grackle = 1; // chemistry on - pack.my_chemistry->use_isrf_field = 1; - pack.my_chemistry->with_radiative_cooling = 1; // cooling on - pack.my_chemistry->primordial_chemistry = primordial_chemistry; - pack.my_chemistry->dust_chemistry = (primordial_chemistry == 0) ? 0 : 1; - pack.my_chemistry->metal_cooling = 1; // metal cooling on - pack.my_chemistry->UVbackground = 1; // UV background on - - pack.successful_data_file = grtest::set_standard_datafile( - *pack.my_chemistry, "CloudyData_UVB=HM2012.h5" - ); - if (!pack.successful_data_file) { return pack; } - - // Finally, initialize the chemistry object. - if (initialize_chemistry_data(&pack.my_units) != GR_SUCCESS) { return pack; } - pack.successful_init = true; - return pack; -} - -} // anonymous namespace - // this defines a parameterized test-fixture (it is parameterized on -// primordial_chemistry) +// InitStatus::data_file_not_found) // -> it has a GetParam() method to access the parameters // -> to assist with avoiding memory leaks, I decided to also make this setup // and teardown GrackleCtxPack. @@ -324,30 +258,25 @@ GrackleCtxPack setup_simple_grackle_conditions(int primordial_chemistry) { // really doesn't care how grackle is configured (other than that // primordial_chemistry varies and that it will actually perform // calculations) -class APIConventionTest : public testing::TestWithParam { +class APIConventionTest : public testing::TestWithParam { protected: void SetUp() override { // Disable output grackle_verbose = 0; // called immediately after the constructor (but before the test-case) - int primordial_chemistry = GetParam(); - - pack_ = setup_simple_grackle_conditions(primordial_chemistry); - if (!pack_.successful_default) { - FAIL() << "Error in set_default_chemistry_parameters."; - } else if (!pack_.successful_data_file) { - GTEST_SKIP() << "something went wrong with finding the data file"; - } else if (!pack_.successful_init) { - FAIL() << "Error in initialize_chemistry_data."; + grtest::InitStatus status; + pack_ = grtest::GrackleCtxPack::create(GetParam(), &status); + if (!pack_.is_initialized()) { + if (status == grtest::InitStatus::datafile_notfound) { + GTEST_SKIP() << "something went wrong with finding the data file"; + } else { + FAIL() << "Error in initialize_chemistry_data."; + } } } - void TearDown() override { - cleanup_grackle_conditions(this->pack_); - } - - GrackleCtxPack pack_; + grtest::GrackleCtxPack pack_; }; TEST_P(APIConventionTest, GridZoneStartEnd) { @@ -355,7 +284,8 @@ TEST_P(APIConventionTest, GridZoneStartEnd) { grid_props my_grid_props = {{5,6,7}, {1,0,2}}; // alias the pack_ attribute tracked by the fixture - GrackleCtxPack& pack = pack_; + grtest::GrackleCtxPack& pack = pack_; + code_units my_units = pack_.initial_units(); // initialize pseudo random number generator std::uint32_t seed = 1379069008; @@ -377,7 +307,7 @@ TEST_P(APIConventionTest, GridZoneStartEnd) { // For example: my_fields.density = my_field_map["density"].data() val_vec_map_t my_field_map; construct_field_data( - my_fields, my_grid_props, pack.my_units, my_field_map, generator + my_fields, my_grid_props, pack.my_chemistry(), my_units, my_field_map, generator ); // orig_field_map_copy is a deepcopy of my_field_map. We will use this as a @@ -393,9 +323,11 @@ TEST_P(APIConventionTest, GridZoneStartEnd) { // Evolving the chemistry. // some timestep - double dt = 3.15e7 * 1e6 / pack.my_units.time_units; + double dt = 3.15e7 * 1e6 / my_units.time_units; - if (solve_chemistry(&pack.my_units, &my_fields, dt) != GR_SUCCESS) { + if (local_solve_chemistry(pack.my_chemistry(), pack.my_rates(), + &my_units, &my_fields, + dt)!= GR_SUCCESS) { FAIL() << "Error running solve_chemistry"; } @@ -404,17 +336,17 @@ TEST_P(APIConventionTest, GridZoneStartEnd) { FAIL() << "Some ghost values were modified in solve_chemistry."; } - // Now check what hapens when computing various properties - const char* func_names[5] = {"calculate_cooling_time", - "calculate_temperature", - "calculate_pressure", - "calculate_gamma", - "calculate_dust_temperature"}; - property_func func_ptrs[5] = {&calculate_cooling_time, - &calculate_temperature, - &calculate_pressure, - &calculate_gamma, - &calculate_dust_temperature}; + // Now check what happens when computing various properties + const char* func_names[5] = {"local_calculate_cooling_time", + "local_calculate_temperature", + "local_calculate_pressure", + "local_calculate_gamma", + "local_calculate_dust_temperature"}; + property_func func_ptrs[5] = {&local_calculate_cooling_time, + &local_calculate_temperature, + &local_calculate_pressure, + &local_calculate_gamma, + &local_calculate_dust_temperature}; for (int i = 0; i < 5; i++){ std::uint32_t seed2 = 1860889605; std::minstd_rand generator2(seed2); @@ -426,7 +358,7 @@ TEST_P(APIConventionTest, GridZoneStartEnd) { // perform the calculation property_func func_ptr = func_ptrs[i]; - if ( (*func_ptr)(&pack.my_units, &my_fields, out_vals.data()) + if ( (*func_ptr)(pack.my_chemistry(), pack.my_rates(), &my_units, &my_fields, out_vals.data()) != GR_SUCCESS ) { FAIL() << "Error reported by " << func_names; } @@ -439,6 +371,15 @@ TEST_P(APIConventionTest, GridZoneStartEnd) { } +using grtest::FullConfPreset; +using grtest::ChemPreset; +using grtest::InitialUnitPreset; + INSTANTIATE_TEST_SUITE_P( - VaryingPrimordialChem, APIConventionTest, ::testing::Range(0, 4) + VaryingPrimordialChem, APIConventionTest, + ::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}) ); From c4495e7a9cd8bdd0423601b23432e06beb3f4688 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 16 Dec 2025 16:55:57 -0500 Subject: [PATCH 08/19] transfer grtest_api_fixture out of test_ghost_zone --- tests/grtestutils/googletest/fixtures.hpp | 96 ++++++----------------- tests/grtestutils/preset.hpp | 80 +++++++++++++++++++ tests/unit/test_ghost_zone.cpp | 35 +-------- 3 files changed, 107 insertions(+), 104 deletions(-) diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index b1bc74969..e139ed072 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -25,84 +25,34 @@ namespace grtest { -/// Tracks the group of Grackle objects needed for executing API functions +/// defines a parameterized test-fixture that can used to run a parametrized +/// set of tests that are initialized with different chemistry data presets. /// -/// The primary motivation for this object's existence is making sure that -/// the allocations get cleaned up when a test fails +/// This sets up a GrackleCtxPack (where the contents are configured with +/// appropriate presets) and deallocates that memory at the end of the test. /// -/// @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; +/// How To Use +/// ========== +/// To make use of this, you might create a subclass of this type that is named +/// for the test suite. I don't love this strategy, but it seems to be the +/// standard way to do things. We can revisit this in the future. +class ParametrizedConfigPresetFixture + : public testing::TestWithParam { +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::datafile_notfound) { + GTEST_SKIP() << "something went wrong with finding the data file"; + } else { + FAIL() << "Error in initialize_chemistry_data."; } - 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; } + + GrackleCtxPack pack_; }; } // namespace grtest diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index 5f62c4e7b..c8a22a526 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -61,6 +61,86 @@ 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 diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index 2ee3807e5..c565183a6 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -249,37 +249,10 @@ bool equal_ghost_values(val_vec_map_t& ref, val_vec_map_t& actual, return true; } -// this defines a parameterized test-fixture (it is parameterized on -// InitStatus::data_file_not_found) -// -> it has a GetParam() method to access the parameters -// -> to assist with avoiding memory leaks, I decided to also make this setup -// and teardown GrackleCtxPack. -// -> Frankly, I don't love this, but I think it is okay since the test -// really doesn't care how grackle is configured (other than that -// primordial_chemistry varies and that it will actually perform -// calculations) -class APIConventionTest : public testing::TestWithParam { - protected: - void SetUp() override { - // Disable output - grackle_verbose = 0; - - // called immediately after the constructor (but before the test-case) - grtest::InitStatus status; - pack_ = grtest::GrackleCtxPack::create(GetParam(), &status); - if (!pack_.is_initialized()) { - if (status == grtest::InitStatus::datafile_notfound) { - GTEST_SKIP() << "something went wrong with finding the data file"; - } else { - FAIL() << "Error in initialize_chemistry_data."; - } - } - } - - grtest::GrackleCtxPack pack_; -}; +class APIGhostZoneTest: public grtest::ParametrizedConfigPresetFixture +{}; -TEST_P(APIConventionTest, GridZoneStartEnd) { +TEST_P(APIGhostZoneTest, GridZoneStartEnd) { grid_props my_grid_props = {{5,6,7}, {1,0,2}}; @@ -376,7 +349,7 @@ using grtest::ChemPreset; using grtest::InitialUnitPreset; INSTANTIATE_TEST_SUITE_P( - VaryingPrimordialChem, APIConventionTest, + VaryingPrimordialChem, APIGhostZoneTest, ::testing::Values( FullConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, FullConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, From 268b9b4650e03549aeb7f13f707d16d8c9afeb24 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 16 Dec 2025 19:20:53 -0500 Subject: [PATCH 09/19] introduce some basic tests of ratequery --- tests/grtestutils/googletest/fixtures.hpp | 51 +++++++++++++++++++---- tests/grtestutils/preset.cpp | 10 +++++ tests/grtestutils/preset.hpp | 1 + tests/unit/CMakeLists.txt | 11 ++--- tests/unit/test_api_ratequery.cpp | 49 ++++++++++++++++++++++ tests/unit/test_ghost_zone.cpp | 8 ++-- 6 files changed, 113 insertions(+), 17 deletions(-) create mode 100644 tests/unit/test_api_ratequery.cpp diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index e139ed072..e3998dbbe 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -25,7 +25,42 @@ namespace grtest { -/// defines a parameterized test-fixture that can used to run a parametrized +/// test-fixture that can used to run one or more tests with a chemistry data +/// configuration initialized from a given chemistry presets. +/// +/// This sets up a GrackleCtxPack (where the contents are configured with +/// appropriate presets) and deallocates that memory at the end of the test. +/// +/// How To Use +/// ========== +/// To make use of this fixture in a test-suite called `MyFeatureTest`, you +/// need to either: +/// 1. make a type alias (via `using` or `typedef`), named `MyFeatureTest`, of +/// the relevant instantiation of this class template, OR +/// 2. make a subclass, named `MyFeatureTest`, of the relevant instantiation of +/// this class template +template +class ConfigPresetFixture : public testing::Test { +protected: + void SetUp() override { + // called immediately after the constructor (but before the test-case) + + grtest::InitStatus status; + pack = GrackleCtxPack::create(FullConfPreset{chem_preset, unit_preset}, + &status); + if (!pack.is_initialized()) { + if (status == InitStatus::datafile_notfound) { + GTEST_SKIP() << "something went wrong with finding the data file"; + } else { + FAIL() << "Error in initialize_chemistry_data."; + } + } + } + + GrackleCtxPack pack; +}; + +/// defines a parameterized test-fixture that can be used to run a parametrized /// set of tests that are initialized with different chemistry data presets. /// /// This sets up a GrackleCtxPack (where the contents are configured with @@ -33,17 +68,19 @@ namespace grtest { /// /// How To Use /// ========== -/// To make use of this, you might create a subclass of this type that is named -/// for the test suite. I don't love this strategy, but it seems to be the -/// standard way to do things. We can revisit this in the future. +/// To make use of this fixture in a test-suite called `MyFeatureTest`, you +/// need to either: +/// 1. make a type alias (via `using` or `typedef`), named `MyFeatureTest`, of +/// this class, OR +/// 2. make a subclass, named `MyFeatureTest`, of this class class ParametrizedConfigPresetFixture : public testing::TestWithParam { 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()) { + pack = GrackleCtxPack::create(GetParam(), &status); + if (!pack.is_initialized()) { if (status == InitStatus::datafile_notfound) { GTEST_SKIP() << "something went wrong with finding the data file"; } else { @@ -52,7 +89,7 @@ class ParametrizedConfigPresetFixture } } - GrackleCtxPack pack_; + GrackleCtxPack pack; }; } // namespace grtest diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index 22621cc18..53f3a64e3 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -26,6 +26,8 @@ static std::string to_string(const grtest::ChemPreset& preset) { return "pc=2"; case grtest::ChemPreset::primchem3: return "pc=3"; + case grtest::ChemPreset::primchem4_dustspecies3: + return "pc=3-dust_species=4"; } GR_INTERNAL_UNREACHABLE_ERROR(); @@ -68,6 +70,14 @@ grtest::InitStatus grtest::setup_chemistry_data_from_preset( my_chem->dust_chemistry = 1; return 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 = 1; + my_chem->use_dust_density_field = 1; + return InitStatus::success; + } } GR_INTERNAL_UNREACHABLE_ERROR(); } diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index c8a22a526..b30f69632 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -34,6 +34,7 @@ enum class ChemPreset { primchem1, primchem2, primchem3, + primchem4_dustspecies3, }; /// override the settings of my_chem based on the specified preset diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index fd5a75ac1..915a28c93 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -22,11 +22,12 @@ add_executable(runVisitorTests test_visitor.cpp) target_link_libraries(runVisitorTests testdeps) gtest_discover_tests(runVisitorTests) -# one might argue that the following is more of an integration or end-to-end -# test than a unit-test -add_executable(runGhostZoneTests test_ghost_zone.cpp) -target_link_libraries(runGhostZoneTests testdeps) -gtest_discover_tests(runGhostZoneTests) +# tests of the API functions +# -> one might argue that these are better classified as integration or +# end-to-end tests than as unit-test +add_executable(runApiTests test_ghost_zone.cpp test_api_ratequery.cpp) +target_link_libraries(runApiTests testdeps) +gtest_discover_tests(runApiTests) # this target tests that the members of the chemistry_data struct can be # accessed through the "dynamic api." The test cases in this target are diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp new file mode 100644 index 000000000..260d69ba1 --- /dev/null +++ b/tests/unit/test_api_ratequery.cpp @@ -0,0 +1,49 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Define some basic tests of the experimental ratequery API +/// +//===----------------------------------------------------------------------===// + +#include +#include +#include "grtestutils/googletest/fixtures.hpp" + +#include "grackle.h" + +/// returns the rateid used to denote invalid rate names +grunstable_rateid_type get_invalid_rateid(const grtest::GrackleCtxPack& pack) { + // although we don't use pack, yet a forthcoming refactor requires it + return grunstable_ratequery_id(nullptr); +} + +using SimpleRateQueryTest = + grtest::ConfigPresetFixture; + +TEST_F(SimpleRateQueryTest, InvalidIthRate) { + const char* name = grunstable_ith_rate( + std::numeric_limits::max(), nullptr); + EXPECT_EQ(name, nullptr); +} + +TEST_F(SimpleRateQueryTest, EmptyNameQuery) { + grunstable_rateid_type rateid = grunstable_ratequery_id(""); + EXPECT_EQ(rateid, get_invalid_rateid(pack)); +} + +TEST_F(SimpleRateQueryTest, InvalidNameQuery) { + grunstable_rateid_type rateid = grunstable_ratequery_id("NotAValidName"); + EXPECT_EQ(rateid, get_invalid_rateid(pack)); +} + +TEST_F(SimpleRateQueryTest, PtrInvalidRateId) { + double* ptr = + grunstable_ratequery_get_ptr(pack.my_rates(), get_invalid_rateid(pack)); + EXPECT_EQ(ptr, nullptr); +} diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index c565183a6..5bdf4ac8a 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -249,16 +249,14 @@ bool equal_ghost_values(val_vec_map_t& ref, val_vec_map_t& actual, return true; } -class APIGhostZoneTest: public grtest::ParametrizedConfigPresetFixture -{}; +using APIGhostZoneTest = grtest::ParametrizedConfigPresetFixture; TEST_P(APIGhostZoneTest, GridZoneStartEnd) { grid_props my_grid_props = {{5,6,7}, {1,0,2}}; - // alias the pack_ attribute tracked by the fixture - grtest::GrackleCtxPack& pack = pack_; - code_units my_units = pack_.initial_units(); + // the pack attribute holds grtest::GrackleCtxPack + code_units my_units = pack.initial_units(); // initialize pseudo random number generator std::uint32_t seed = 1379069008; From cd7deacef442058e1305c30516bfd65a056aa223 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 16 Dec 2025 21:10:15 -0500 Subject: [PATCH 10/19] add more ratequery tests --- tests/unit/test_api_ratequery.cpp | 112 ++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index 260d69ba1..9033334d6 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -10,7 +10,10 @@ /// //===----------------------------------------------------------------------===// +#include #include +#include +#include #include #include "grtestutils/googletest/fixtures.hpp" @@ -47,3 +50,112 @@ TEST_F(SimpleRateQueryTest, PtrInvalidRateId) { grunstable_ratequery_get_ptr(pack.my_rates(), get_invalid_rateid(pack)); EXPECT_EQ(ptr, nullptr); } + +// will be implemented (in a much more robust manner) in the near future +unsigned long long grunstable_ratequery_nrates( + const chemistry_data_storage* my_rates) { + // current implementation is stupid! (in future, will use my_rates) + unsigned long long i = 0; + while (nullptr != grunstable_ith_rate(i, nullptr)) { + i++; + } + return i; +} + +// most of the remaining tests (and future planned tests) involve iterating +// through all rates made available via the ratequery interface. To make the +// tests themselves as easy to read as possible, we implate a C++-style +// iterator to wrap part of the interface + +struct RateNameIdPair { + std::string name; + grunstable_rateid_type rateid; +}; + +class RQIterator { + chemistry_data_storage* my_rates_; + unsigned long long counter_; + unsigned long long n_rates_; + RateNameIdPair pair_; + + RQIterator& update_pair_and_ret_(unsigned long long val) { + if (val < n_rates_) { + pair_.name = std::string(grunstable_ith_rate(val, &pair_.rateid)); + } + return *this; + } + +public: + using iterator_category = std::input_iterator_tag; + using value_type = RateNameIdPair; + using difference_type = std::ptrdiff_t; + using pointer = const RateNameIdPair*; + using reference = const RateNameIdPair; + + RQIterator(unsigned long long counter, unsigned long long n_rates, + chemistry_data_storage* my_rates) + : my_rates_(my_rates), counter_(counter), n_rates_(n_rates) { + update_pair_and_ret_(counter); + } + + bool operator==(RQIterator other) const { + return (counter_ == other.counter_) && (my_rates_ == other.my_rates_); + } + + bool operator!=(RQIterator other) const { return !(*this == other); } + reference operator*() const { return pair_; } + RQIterator& operator++() { return update_pair_and_ret_(++counter_); } + + RQIterator operator++(int) { + RQIterator ret = *this; + ++(*this); + return ret; + } +}; + +// used for creating the iterator and within range-based for-loops +class RateQueryRange { + grtest::GrackleCtxPack& pack_; + long long n_rates_; + +public: + explicit RateQueryRange(grtest::GrackleCtxPack& pack) + : pack_(pack), n_rates_(grunstable_ratequery_nrates(pack.my_rates())) {} + + RQIterator begin() { return RQIterator(0, n_rates_, pack_.my_rates()); } + RQIterator end() { return RQIterator(n_rates_, n_rates_, pack_.my_rates()); } +}; + +using ParametrizedRateQueryTest = grtest::ParametrizedConfigPresetFixture; + +TEST_P(ParametrizedRateQueryTest, AllUnique) { + std::set name_set; + std::set id_set; + for (const RateNameIdPair pair : RateQueryRange(pack)) { + ASSERT_TRUE(name_set.insert(pair.name).second) + << "the name, \"" << pair.name << "\" appears more than once"; + ASSERT_TRUE(id_set.insert(pair.rateid).second) + << "the id, " << pair.rateid << " appears more than once"; + } +} + +TEST_P(ParametrizedRateQueryTest, ConsistentIDs) { + for (const RateNameIdPair pair : RateQueryRange(pack)) { + grunstable_rateid_type rateid = grunstable_ratequery_id(pair.name.c_str()); + EXPECT_EQ(rateid, pair.rateid); + } +} + +using grtest::ChemPreset; +using grtest::FullConfPreset; +using grtest::InitialUnitPreset; + +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})); From 24257017a30d3c805894e966c81b3bc50824d5f2 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 16 Dec 2025 21:11:49 -0500 Subject: [PATCH 11/19] tweak the name of the test_ghost_zone tests --- tests/unit/test_ghost_zone.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index 5bdf4ac8a..16628721f 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -347,7 +347,7 @@ using grtest::ChemPreset; using grtest::InitialUnitPreset; INSTANTIATE_TEST_SUITE_P( - VaryingPrimordialChem, APIGhostZoneTest, + /* 1st arg is intentionally empty */, APIGhostZoneTest, ::testing::Values( FullConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, FullConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, From 37fad17d0a47c5822aa05af0bd578657900521c3 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 17:01:18 -0500 Subject: [PATCH 12/19] factor out machinery for creating adaptor --- tests/grtestutils/CMakeLists.txt | 1 + tests/grtestutils/iterator_adaptor.hpp | 140 +++++++++++++++++++++++++ tests/unit/test_api_ratequery.cpp | 81 ++------------ 3 files changed, 147 insertions(+), 75 deletions(-) create mode 100644 tests/grtestutils/iterator_adaptor.hpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index f05d4ff1f..27bf11d17 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 + iterator_adaptor.hpp os.hpp os.cpp preset.hpp preset.cpp utils.hpp utils.cpp diff --git a/tests/grtestutils/iterator_adaptor.hpp b/tests/grtestutils/iterator_adaptor.hpp new file mode 100644 index 000000000..ad84a3559 --- /dev/null +++ b/tests/grtestutils/iterator_adaptor.hpp @@ -0,0 +1,140 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Declare and implement the IteratorAdaptor +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_ITERATOR_ADAPTOR_HPP +#define GRTESTUTILS_ITERATOR_ADAPTOR_HPP + +#include +#include + +#include "grackle.h" +#include "preset.hpp" + +namespace grtest { + +/// the standard value-type that an IteratorAdaptor instantiation refers to +struct NameIdPair { + std::string name; + long long id; +}; + +/// implements a C++ style InputIterator by adapting a simple Plugin type +/// that wraps a set of Grackle functions +/// +/// This is useful for making use of C++ standard library algorithms and +/// (arguably more importantly) making use of range-based for-loops +template +class IteratorAdaptor { + unsigned long long counter_; + unsigned long long n_rates_; + Plugin plugin_; + NameIdPair current_pair_; + + /// Updates current_pair_ and returns `*this` + IteratorAdaptor& update_pair_and_ret_(unsigned long long current_count) { + if (current_count < this->n_rates_) { + this->current_pair_ = this->plugin_(current_count); + } + return *this; + } + +public: + using iterator_category = std::input_iterator_tag; + using value_type = NameIdPair; + using difference_type = std::ptrdiff_t; + using pointer = const NameIdPair*; + using reference = const NameIdPair; + + /// construct a new instance + IteratorAdaptor(unsigned long long counter, unsigned long long n_rates, + Plugin plugin) + : counter_(counter), n_rates_(n_rates), plugin_(plugin) { + update_pair_and_ret_(counter); + } + + /// implements the equality operation + bool operator==(const IteratorAdaptor& other) const { + return (counter_ == other.counter_) && (plugin_ == other.plugin_); + } + + /// implements the inequality operation + bool operator!=(const IteratorAdaptor& other) const { + return !(*this == other); + } + + /// implements the dereference operation + reference operator*() const { return current_pair_; } + + /// implements the prefix increment operation + /// + /// This effectively implements `++x`, which increments the value of `x` + /// before determining the returned value. In other words, `++x` returns the + /// value of `x` from **after** after the increment + IteratorAdaptor& operator++() { return update_pair_and_ret_(++counter_); } + + /// implements the prefix increment operation + /// + /// This effectively implements `x++`, which increments the value of `x` + /// after determining the returned value. In other words, `x++` returns the + /// value of `x` from **before** the increment + IteratorAdaptor operator++(int) { + IteratorAdaptor ret = *this; + ++(*this); + return ret; + } +}; + +// Now lets use this machinery to implement logic iterating over the names +// accessible through the ratequery api + +struct RateQueryPlugin { + chemistry_data_storage* my_rates; + + NameIdPair operator()(unsigned long long i) const { + grunstable_rateid_type tmp; + const char* name = grunstable_ith_rate(i, &tmp); + return NameIdPair{name, tmp}; + } + + bool operator==(const RateQueryPlugin& other) const { + return my_rates == other.my_rates; + } +}; + +// will be implemented (in a much more robust manner) in the near future +inline unsigned long long grunstable_ratequery_nrates( + const chemistry_data_storage* my_rates) { + // current implementation is stupid! (in future, will use my_rates) + unsigned long long i = 0; + while (nullptr != grunstable_ith_rate(i, nullptr)) { + i++; + } + return i; +} + +/// used for creating the iterator and within range-based for-loops +class RateQueryRange { + RateQueryPlugin plugin_; + using iterator = IteratorAdaptor; + long long n_rates_; + +public: + explicit RateQueryRange(grtest::GrackleCtxPack& pack) + : plugin_(RateQueryPlugin{pack.my_rates()}), + n_rates_(grunstable_ratequery_nrates(pack.my_rates())) {} + + iterator begin() { return iterator(0, n_rates_, plugin_); } + iterator end() { return iterator(n_rates_, n_rates_, plugin_); } +}; + +} // namespace grtest + +#endif // GRTESTUTILS_ITERATOR_ADAPTOR_HPP diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index 9033334d6..02d0882bd 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -15,6 +15,7 @@ #include #include #include +#include "grtestutils/iterator_adaptor.hpp" #include "grtestutils/googletest/fixtures.hpp" #include "grackle.h" @@ -51,98 +52,28 @@ TEST_F(SimpleRateQueryTest, PtrInvalidRateId) { EXPECT_EQ(ptr, nullptr); } -// will be implemented (in a much more robust manner) in the near future -unsigned long long grunstable_ratequery_nrates( - const chemistry_data_storage* my_rates) { - // current implementation is stupid! (in future, will use my_rates) - unsigned long long i = 0; - while (nullptr != grunstable_ith_rate(i, nullptr)) { - i++; - } - return i; -} - // most of the remaining tests (and future planned tests) involve iterating // through all rates made available via the ratequery interface. To make the // tests themselves as easy to read as possible, we implate a C++-style // iterator to wrap part of the interface -struct RateNameIdPair { - std::string name; - grunstable_rateid_type rateid; -}; - -class RQIterator { - chemistry_data_storage* my_rates_; - unsigned long long counter_; - unsigned long long n_rates_; - RateNameIdPair pair_; - - RQIterator& update_pair_and_ret_(unsigned long long val) { - if (val < n_rates_) { - pair_.name = std::string(grunstable_ith_rate(val, &pair_.rateid)); - } - return *this; - } - -public: - using iterator_category = std::input_iterator_tag; - using value_type = RateNameIdPair; - using difference_type = std::ptrdiff_t; - using pointer = const RateNameIdPair*; - using reference = const RateNameIdPair; - - RQIterator(unsigned long long counter, unsigned long long n_rates, - chemistry_data_storage* my_rates) - : my_rates_(my_rates), counter_(counter), n_rates_(n_rates) { - update_pair_and_ret_(counter); - } - - bool operator==(RQIterator other) const { - return (counter_ == other.counter_) && (my_rates_ == other.my_rates_); - } - - bool operator!=(RQIterator other) const { return !(*this == other); } - reference operator*() const { return pair_; } - RQIterator& operator++() { return update_pair_and_ret_(++counter_); } - - RQIterator operator++(int) { - RQIterator ret = *this; - ++(*this); - return ret; - } -}; - -// used for creating the iterator and within range-based for-loops -class RateQueryRange { - grtest::GrackleCtxPack& pack_; - long long n_rates_; - -public: - explicit RateQueryRange(grtest::GrackleCtxPack& pack) - : pack_(pack), n_rates_(grunstable_ratequery_nrates(pack.my_rates())) {} - - RQIterator begin() { return RQIterator(0, n_rates_, pack_.my_rates()); } - RQIterator end() { return RQIterator(n_rates_, n_rates_, pack_.my_rates()); } -}; - using ParametrizedRateQueryTest = grtest::ParametrizedConfigPresetFixture; TEST_P(ParametrizedRateQueryTest, AllUnique) { std::set name_set; std::set id_set; - for (const RateNameIdPair pair : RateQueryRange(pack)) { + for (const grtest::NameIdPair pair : grtest::RateQueryRange(pack)) { ASSERT_TRUE(name_set.insert(pair.name).second) << "the name, \"" << pair.name << "\" appears more than once"; - ASSERT_TRUE(id_set.insert(pair.rateid).second) - << "the id, " << pair.rateid << " appears more than once"; + ASSERT_TRUE(id_set.insert(pair.id).second) + << "the id, " << pair.id << " appears more than once"; } } TEST_P(ParametrizedRateQueryTest, ConsistentIDs) { - for (const RateNameIdPair pair : RateQueryRange(pack)) { + for (const grtest::NameIdPair pair : grtest::RateQueryRange(pack)) { grunstable_rateid_type rateid = grunstable_ratequery_id(pair.name.c_str()); - EXPECT_EQ(rateid, pair.rateid); + EXPECT_EQ(rateid, pair.id); } } From 39e1c0c7d8183b53c366f033e69b7a4fe95e8ec7 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 19 Dec 2025 08:14:21 -0500 Subject: [PATCH 13/19] fix a broken header include --- tests/grtestutils/googletest/fixtures.hpp | 3 --- tests/grtestutils/preset.hpp | 1 + 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index e3998dbbe..25c53bb22 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -18,9 +18,6 @@ // 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 - -#include "grackle.h" #include "../preset.hpp" namespace grtest { diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index b30f69632..6f942167d 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -13,6 +13,7 @@ #define GRTESTUTILS_PRESET_HPP #include "grackle.h" +#include #include #include From 6ad2a0b53f1729be851e7da1ca5ca4781bed5739 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 20:40:05 -0500 Subject: [PATCH 14/19] Introduce tests before converting dynamic_api.c In the process, I actually identified and fixed a minor bug --- src/clib/dynamic_api.c | 4 +- tests/grtestutils/iterator_adaptor.hpp | 54 +++++- tests/unit/CMakeLists.txt | 3 +- tests/unit/test_dynamic_chem_param_api.cpp | 197 +++++++++++++++++++++ 4 files changed, 255 insertions(+), 3 deletions(-) create mode 100644 tests/unit/test_dynamic_chem_param_api.cpp diff --git a/src/clib/dynamic_api.c b/src/clib/dynamic_api.c index 7d3d76d95..154e3c2be 100644 --- a/src/clib/dynamic_api.c +++ b/src/clib/dynamic_api.c @@ -125,7 +125,9 @@ const char* param_name_string(unsigned int i) // function to query the number of parameters of chemistry_data of a given type unsigned int grackle_num_params(const char* type_name){ - if (strcmp(type_name, "int") == 0) { + if (type_name == NULL) { + return 0; + } else if (strcmp(type_name, "int") == 0) { return (unsigned int)_int_param_l.len; } else if (strcmp(type_name, "double") == 0) { return (unsigned int) _double_param_l.len; diff --git a/tests/grtestutils/iterator_adaptor.hpp b/tests/grtestutils/iterator_adaptor.hpp index ad84a3559..61f3ba517 100644 --- a/tests/grtestutils/iterator_adaptor.hpp +++ b/tests/grtestutils/iterator_adaptor.hpp @@ -19,7 +19,6 @@ #include "preset.hpp" namespace grtest { - /// the standard value-type that an IteratorAdaptor instantiation refers to struct NameIdPair { std::string name; @@ -137,4 +136,57 @@ class RateQueryRange { } // namespace grtest +// Now lets use this machinery to implement logic for iterating over the names +// of chemistry parameters +extern "C" { +typedef const char* param_name_fn(unsigned int); +} + +namespace grtest { + +struct ParamItrPlugin { + param_name_fn* fn; + + NameIdPair operator()(unsigned long long i) const { + return {fn(static_cast(i)), static_cast(i)}; + } + + bool operator==(const ParamItrPlugin& other) const { return fn == other.fn; } +}; + +enum class ChemParamType { INT, DOUBLE, STRING }; + +/// used for creating the iterator and within range-based for-loops +class ChemParamRange { + using iterator = IteratorAdaptor; + ParamItrPlugin plugin_; + long long n_rates_; + +public: + explicit ChemParamRange(grtest::ChemParamType param) { + switch (param) { + case ChemParamType::INT: + n_rates_ = grackle_num_params("int"); + plugin_ = ParamItrPlugin{¶m_name_int}; + break; + case ChemParamType::DOUBLE: + n_rates_ = grackle_num_params("double"); + plugin_ = ParamItrPlugin{¶m_name_double}; + break; + case ChemParamType::STRING: + n_rates_ = grackle_num_params("string"); + plugin_ = ParamItrPlugin{¶m_name_string}; + break; + default: // should NEVER come up (but the behavior is well-defined) + n_rates_ = 0; + plugin_ = ParamItrPlugin{nullptr}; + } + } + + iterator begin() { return iterator(0, n_rates_, plugin_); } + iterator end() { return iterator(n_rates_, n_rates_, plugin_); } +}; + +} // namespace grtest + #endif // GRTESTUTILS_ITERATOR_ADAPTOR_HPP diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 915a28c93..9a6e8df9c 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -25,7 +25,8 @@ gtest_discover_tests(runVisitorTests) # tests of the API functions # -> one might argue that these are better classified as integration or # end-to-end tests than as unit-test -add_executable(runApiTests test_ghost_zone.cpp test_api_ratequery.cpp) +add_executable(runApiTests + test_ghost_zone.cpp test_api_ratequery.cpp test_dynamic_chem_param_api.cpp) target_link_libraries(runApiTests testdeps) gtest_discover_tests(runApiTests) diff --git a/tests/unit/test_dynamic_chem_param_api.cpp b/tests/unit/test_dynamic_chem_param_api.cpp new file mode 100644 index 000000000..6d37e4356 --- /dev/null +++ b/tests/unit/test_dynamic_chem_param_api.cpp @@ -0,0 +1,197 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Test the API for dynamically querying `chemistry_data` +/// +//===----------------------------------------------------------------------===// + +#include "grackle.h" +#include "grtestutils/iterator_adaptor.hpp" + +#include + +#include +#include +#include +#include +#include +#include + +TEST(DynamicChemParam, EmptyStrNumParams) { + EXPECT_EQ(grackle_num_params(""), 0); +} + +TEST(DynamicChemParam, NullptrNumParams) { + EXPECT_EQ(grackle_num_params(nullptr), 0); +} + +TEST(DynamicChemParam, NotATypeNumParams) { + EXPECT_EQ(grackle_num_params("NotARealType"), 0); +} + +TEST(DynamicChemParam, ParamNameIndexZero) { + EXPECT_NE(param_name_int(0u), nullptr); + EXPECT_NE(param_name_double(0u), nullptr); + EXPECT_NE(param_name_string(0u), nullptr); +} + +TEST(DynamicChemParam, ParamNameIndexMaxNumber) { + EXPECT_NE(param_name_int(grackle_num_params("int") - 1u), nullptr); + EXPECT_NE(param_name_double(grackle_num_params("double") - 1u), nullptr); + EXPECT_NE(param_name_string(grackle_num_params("string") - 1u), nullptr); +} + +TEST(DynamicChemParam, ParamNameIndexPassCount) { + EXPECT_EQ(param_name_int(grackle_num_params("int")), nullptr); + EXPECT_EQ(param_name_double(grackle_num_params("double")), nullptr); + EXPECT_EQ(param_name_string(grackle_num_params("string")), nullptr); +} + +TEST(DynamicChemParam, ParamNameIndexPassCountPlus1) { + EXPECT_EQ(param_name_int(grackle_num_params("int") + 1u), nullptr); + EXPECT_EQ(param_name_double(grackle_num_params("double") + 1u), nullptr); + EXPECT_EQ(param_name_string(grackle_num_params("string") + 1u), nullptr); +} + +TEST(DynamicChemParam, ParamNameMaximum) { + unsigned i = std::numeric_limits::max(); + + EXPECT_EQ(param_name_int(i), nullptr); + EXPECT_EQ(param_name_double(i), nullptr); + EXPECT_EQ(param_name_string(i), nullptr); +} + +static constexpr std::array known_chem_types = { + grtest::ChemParamType::INT, grtest::ChemParamType::DOUBLE, + grtest::ChemParamType::STRING}; + +TEST(DynamicChemParam, UniqueNames) { + // validate that all parameter names are unique + std::set name_set; + + for (const grtest::ChemParamType type : known_chem_types) { + // iterate over all parameter names of the current type + for (const auto [param_name, _] : grtest::ChemParamRange(type)) { + ASSERT_TRUE(name_set.insert(param_name).second) + << "the name, \"" << param_name << "\" appears more than once"; + } + } +} + +TEST(DynamicChemParam, AccessNullChemistryData) { + EXPECT_EQ(local_chemistry_data_access_int(nullptr, "use_grackle"), nullptr); + EXPECT_EQ(local_chemistry_data_access_double(nullptr, "Gamma"), nullptr); + EXPECT_EQ(local_chemistry_data_access_string(nullptr, "grackle_data_file"), + nullptr); +} + +TEST(DynamicChemParam, AccessEmptyString) { + chemistry_data my_chemistry; + ASSERT_EQ(local_initialize_chemistry_parameters(&my_chemistry), GR_SUCCESS); + + const char* name = ""; + + EXPECT_EQ(local_chemistry_data_access_int(&my_chemistry, name), nullptr); + EXPECT_EQ(local_chemistry_data_access_double(&my_chemistry, name), nullptr); + EXPECT_EQ(local_chemistry_data_access_string(&my_chemistry, name), nullptr); +} + +TEST(DynamicChemParam, AccessNotAParam) { + chemistry_data my_chemistry; + ASSERT_EQ(local_initialize_chemistry_parameters(&my_chemistry), GR_SUCCESS); + + const char* name = "NotARealParameter"; + + EXPECT_EQ(local_chemistry_data_access_int(&my_chemistry, name), nullptr); + EXPECT_EQ(local_chemistry_data_access_double(&my_chemistry, name), nullptr); + EXPECT_EQ(local_chemistry_data_access_string(&my_chemistry, name), nullptr); +} + +static void* access_ptr_(chemistry_data* my_chem, grtest::ChemParamType type, + const char* param_name) { + switch (type) { + case grtest::ChemParamType::INT: { + return (void*)local_chemistry_data_access_int(my_chem, param_name); + } + case grtest::ChemParamType::DOUBLE: { + return (void*)local_chemistry_data_access_double(my_chem, param_name); + } + case grtest::ChemParamType::STRING: { + return (void*)local_chemistry_data_access_string(my_chem, param_name); + } + default: + return nullptr; + } +} + +TEST(DynamicChemParam, UniqueAddresses) { + chemistry_data my_chem; + ASSERT_EQ(local_initialize_chemistry_parameters(&my_chem), GR_SUCCESS); + + // we will validate that all parameter names are successfully access addresses + // with different names + const std::map map({ + {grtest::ChemParamType::INT, "int"}, + {grtest::ChemParamType::DOUBLE, "double"}, + {grtest::ChemParamType::STRING, "string"}, + }); + std::set address_set; + + for (const grtest::ChemParamType type : known_chem_types) { + // iterate over all parameter names of the current type + for (const auto [param_name, _] : grtest::ChemParamRange(type)) { + void* ptr = access_ptr_(&my_chem, type, param_name.c_str()); + EXPECT_NE(ptr, nullptr) + << "local_chemistry_data_access_" << map.at(type) + << " returned nullptr for the " << map.at(type) << "-typed " + << "parameter, \"" << param_name << '"'; + + uintptr_t address = reinterpret_cast(ptr); + ASSERT_TRUE(address_set.insert(address).second) + << "address associated with, \"" << param_name << "\" appears more " + << "than once"; + } + } +} + +TEST(DynamicChemParam, AccessWrongType) { + chemistry_data my_chem; + ASSERT_EQ(local_initialize_chemistry_parameters(&my_chem), GR_SUCCESS); + + for (const grtest::ChemParamType type : known_chem_types) { + // iterate over all parameter names of the current type + for (const auto [param_name, _] : grtest::ChemParamRange(type)) { + if (type != grtest::ChemParamType::INT) { + int* ptr = + local_chemistry_data_access_int(&my_chem, param_name.c_str()); + EXPECT_EQ(ptr, nullptr) + << "local_chemistry_data_access_int should " + << "return a nullptr when called to access a non-integer " + << "parameter, like \"" << param_name << '"'; + } + + if (type != grtest::ChemParamType::DOUBLE) { + double* ptr = + local_chemistry_data_access_double(&my_chem, param_name.c_str()); + EXPECT_EQ(ptr, nullptr) + << "local_chemistry_data_access_double should " + << "return a nullptr when called to access a non-double " + << "parameter, like \"" << param_name << '"'; + } + + if (type != grtest::ChemParamType::STRING) { + char** ptr = + local_chemistry_data_access_string(&my_chem, param_name.c_str()); + EXPECT_EQ(ptr, nullptr) + << "local_chemistry_data_access_string should " + << "return a nullptr when called to access a non-string " + << "parameter, like \"" << param_name << '"'; + } + } + } +} From 5074cc4bae5cd1a2f5213b3b2402dc13dd7d44f4 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 20:46:25 -0500 Subject: [PATCH 15/19] make names of variables/macros for C++ compatible --- src/clib/dynamic_api.c | 60 +++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/src/clib/dynamic_api.c b/src/clib/dynamic_api.c index 154e3c2be..b0bea3317 100644 --- a/src/clib/dynamic_api.c +++ b/src/clib/dynamic_api.c @@ -20,7 +20,7 @@ #include #include "grackle_chemistry_data.h" -// initialize _int_param_l, _double_param_l & _string_param_l. They are sized +// initialize int_param_l_, double_param_l_ & string_param_l_. They are sized // lists that respectively hold entries for each int, double, and string field // in chemistry_data. These are only used internally typedef struct { const size_t offset; const char * name; } param_entry; @@ -28,38 +28,38 @@ typedef struct { const size_t len; const param_entry * entries; } param_list; #define INIT_PAR_LIST(ENTRIES) {sizeof(ENTRIES) / sizeof(param_entry), ENTRIES} -#define _LIST_INT_INT(FIELD) {offsetof(chemistry_data, FIELD), #FIELD}, -#define _LIST_INT_DOUBLE(FIELD) /* ... */ -#define _LIST_INT_STRING(FIELD) /* ... */ +#define LIST_INT_INT(FIELD) {offsetof(chemistry_data, FIELD), #FIELD}, +#define LIST_INT_DOUBLE(FIELD) /* ... */ +#define LIST_INT_STRING(FIELD) /* ... */ -#define _LIST_DOUBLE_INT(FIELD) /* ... */ -#define _LIST_DOUBLE_DOUBLE(FIELD) {offsetof(chemistry_data, FIELD), #FIELD}, -#define _LIST_DOUBLE_STRING(FIELD) /* ... */ +#define LIST_DOUBLE_INT(FIELD) /* ... */ +#define LIST_DOUBLE_DOUBLE(FIELD) {offsetof(chemistry_data, FIELD), #FIELD}, +#define LIST_DOUBLE_STRING(FIELD) /* ... */ -#define _LIST_STRING_INT(FIELD) /* ... */ -#define _LIST_STRING_DOUBLE(FIELD) /* ... */ -#define _LIST_STRING_STRING(FIELD) {offsetof(chemistry_data, FIELD), #FIELD}, +#define LIST_STRING_INT(FIELD) /* ... */ +#define LIST_STRING_DOUBLE(FIELD) /* ... */ +#define LIST_STRING_STRING(FIELD) {offsetof(chemistry_data, FIELD), #FIELD}, -static const param_entry _int_param_entries[] = { - #define ENTRY(FIELD, TYPE, DEFAULT_VAL) _LIST_INT_ ## TYPE(FIELD) +static const param_entry int_param_entries_[] = { + #define ENTRY(FIELD, TYPE, DEFAULT_VAL) LIST_INT_ ## TYPE(FIELD) #include "grackle_chemistry_data_fields.def" #undef ENTRY }; -static const param_list _int_param_l = INIT_PAR_LIST(_int_param_entries); +static const param_list int_param_l_ = INIT_PAR_LIST(int_param_entries_); -static const param_entry _double_param_entries[] = { - #define ENTRY(FIELD, TYPE, DEFAULT_VAL) _LIST_DOUBLE_ ## TYPE(FIELD) +static const param_entry double_param_entries_[] = { + #define ENTRY(FIELD, TYPE, DEFAULT_VAL) LIST_DOUBLE_ ## TYPE(FIELD) #include "grackle_chemistry_data_fields.def" #undef ENTRY }; -static const param_list _double_param_l = INIT_PAR_LIST(_double_param_entries); +static const param_list double_param_l_ = INIT_PAR_LIST(double_param_entries_); -static const param_entry _string_param_entries[] = { - #define ENTRY(FIELD, TYPE, DEFAULT_VAL) _LIST_STRING_ ## TYPE(FIELD) +static const param_entry string_param_entries_[] = { + #define ENTRY(FIELD, TYPE, DEFAULT_VAL) LIST_STRING_ ## TYPE(FIELD) #include "grackle_chemistry_data_fields.def" #undef ENTRY }; -static const param_list _string_param_l = INIT_PAR_LIST(_string_param_entries); +static const param_list string_param_l_ = INIT_PAR_LIST(string_param_entries_); // define functions for accessing field values @@ -79,7 +79,7 @@ static const param_list _string_param_l = INIT_PAR_LIST(_string_param_entries); // // This returns a NULL pointer if: my_chemistry is NULL, the field doesn't // exist, or the field doesn't have the specified type -static void* _get_field_ptr(chemistry_data* my_chemistry, const char* name, +static void* get_field_ptr_(chemistry_data* my_chemistry, const char* name, const param_list my_param_l) { if (my_chemistry == NULL) { return NULL; } @@ -95,15 +95,15 @@ static void* _get_field_ptr(chemistry_data* my_chemistry, const char* name, int* local_chemistry_data_access_int(chemistry_data* my_chemistry, const char* param_name) -{ return (int*)_get_field_ptr(my_chemistry, param_name, _int_param_l); } +{ return (int*)get_field_ptr_(my_chemistry, param_name, int_param_l_); } double* local_chemistry_data_access_double(chemistry_data* my_chemistry, const char* param_name) -{ return (double*)_get_field_ptr(my_chemistry, param_name, _double_param_l); } +{ return (double*)get_field_ptr_(my_chemistry, param_name, double_param_l_); } char** local_chemistry_data_access_string(chemistry_data* my_chemistry, const char* param_name) -{ return (char**)_get_field_ptr(my_chemistry, param_name, _string_param_l); } +{ return (char**)get_field_ptr_(my_chemistry, param_name, string_param_l_); } // define functions for accessing the names of chemistry_data: // param_name_int, param_name_double, param_name_string @@ -113,26 +113,26 @@ char** local_chemistry_data_access_string(chemistry_data* my_chemistry, // returns the name of the ``i``th parameter of the specified type. This returns // NULL when there are ``i`` or fewer parameters of the specified type -static const char* _param_name(size_t i, const param_list my_param_list) +static const char* param_name_(size_t i, const param_list my_param_list) { return (i < my_param_list.len) ? my_param_list.entries[i].name : NULL; } const char* param_name_int(unsigned int i) -{return _param_name(i,_int_param_l);} +{return param_name_(i,int_param_l_);} const char* param_name_double(unsigned int i) -{return _param_name(i,_double_param_l);} +{return param_name_(i,double_param_l_);} const char* param_name_string(unsigned int i) -{return _param_name(i,_string_param_l);} +{return param_name_(i,string_param_l_);} // function to query the number of parameters of chemistry_data of a given type unsigned int grackle_num_params(const char* type_name){ if (type_name == NULL) { return 0; } else if (strcmp(type_name, "int") == 0) { - return (unsigned int)_int_param_l.len; + return (unsigned int) int_param_l_.len; } else if (strcmp(type_name, "double") == 0) { - return (unsigned int) _double_param_l.len; + return (unsigned int) double_param_l_.len; } else if (strcmp(type_name, "string") == 0) { - return (unsigned int) _string_param_l.len; + return (unsigned int) string_param_l_.len; } return 0; } From 13abc5088b0131c3e318cd28fd31c1534026e332 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 21:09:39 -0500 Subject: [PATCH 16/19] finished transforming source to be compatible --- src/clib/dynamic_api.c | 49 +++++++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 10 deletions(-) diff --git a/src/clib/dynamic_api.c b/src/clib/dynamic_api.c index b0bea3317..27a9df915 100644 --- a/src/clib/dynamic_api.c +++ b/src/clib/dynamic_api.c @@ -19,26 +19,42 @@ #include #include #include "grackle_chemistry_data.h" +#include "status_reporting.h" + +/// This is collection of enumerators, with an enumerator named for EVERY +/// parameter. Importantly, each enumerator has a unique value (that we use +/// in a giant switch-statement). +enum ParamEnum { +#define ENTRY(FIELD, TYPE, DEFAULT_VAL) ParamEnum_ ## FIELD, +#include "grackle_chemistry_data_fields.def" +#undef ENTRY + + ParamEnum_NUM_ENTRIES // <- always last (so it specifies number of entries) +}; // initialize int_param_l_, double_param_l_ & string_param_l_. They are sized // lists that respectively hold entries for each int, double, and string field // in chemistry_data. These are only used internally -typedef struct { const size_t offset; const char * name; } param_entry; +typedef struct { + const enum ParamEnum enum_val; + const char * name; +} param_entry; + typedef struct { const size_t len; const param_entry * entries; } param_list; #define INIT_PAR_LIST(ENTRIES) {sizeof(ENTRIES) / sizeof(param_entry), ENTRIES} -#define LIST_INT_INT(FIELD) {offsetof(chemistry_data, FIELD), #FIELD}, +#define LIST_INT_INT(FIELD) {ParamEnum_ ## FIELD, #FIELD}, #define LIST_INT_DOUBLE(FIELD) /* ... */ #define LIST_INT_STRING(FIELD) /* ... */ #define LIST_DOUBLE_INT(FIELD) /* ... */ -#define LIST_DOUBLE_DOUBLE(FIELD) {offsetof(chemistry_data, FIELD), #FIELD}, +#define LIST_DOUBLE_DOUBLE(FIELD) {ParamEnum_ ## FIELD, #FIELD}, #define LIST_DOUBLE_STRING(FIELD) /* ... */ #define LIST_STRING_INT(FIELD) /* ... */ #define LIST_STRING_DOUBLE(FIELD) /* ... */ -#define LIST_STRING_STRING(FIELD) {offsetof(chemistry_data, FIELD), #FIELD}, +#define LIST_STRING_STRING(FIELD) {ParamEnum_ ## FIELD, #FIELD}, static const param_entry int_param_entries_[] = { #define ENTRY(FIELD, TYPE, DEFAULT_VAL) LIST_INT_ ## TYPE(FIELD) @@ -75,22 +91,35 @@ static const param_list string_param_l_ = INIT_PAR_LIST(string_param_entries_); // possible (it would just involve more code). -// retrieves a pointer to the field of my_chemistry that's named ``name``. -// -// This returns a NULL pointer if: my_chemistry is NULL, the field doesn't -// exist, or the field doesn't have the specified type +/// retrieves a pointer to the field of my_chemistry that's named ``name``. +/// +/// This returns a NULL pointer if: my_chemistry is NULL, the field doesn't +/// exist, or the field doesn't have the specified type static void* get_field_ptr_(chemistry_data* my_chemistry, const char* name, const param_list my_param_l) { if (my_chemistry == NULL) { return NULL; } + // lookup the enum value associated with name + enum ParamEnum enum_val = ParamEnum_NUM_ENTRIES; for (size_t param_index = 0; param_index < my_param_l.len; param_index++){ const param_entry* entry = my_param_l.entries + param_index; if (strcmp(entry->name, name) == 0) { - return (void*)( (char*)my_chemistry + entry->offset ); + enum_val = entry->enum_val; + break; } } - return NULL; + + // now use a switch statement to actually return the appropriate pointer + switch (enum_val) { + #define ENTRY(FIELD, TYPE, DEFAULT_VAL) \ + case ParamEnum_ ## FIELD: { return (void*)(&my_chemistry->FIELD); } + #include "grackle_chemistry_data_fields.def" + #undef ENTRY + + case ParamEnum_NUM_ENTRIES: return NULL; // <- unknown name + } + GR_INTERNAL_UNREACHABLE_ERROR(); } int* local_chemistry_data_access_int(chemistry_data* my_chemistry, From c5c113cc56176635917b94ffacd4a3638449add4 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 21:17:58 -0500 Subject: [PATCH 17/19] convert src/clib/dynamic_api.{c->cpp} --- .clang-format-ignore | 2 +- src/clib/CMakeLists.txt | 2 +- src/clib/{dynamic_api.c => dynamic_api.cpp} | 62 +++++++++++++-------- 3 files changed, 41 insertions(+), 25 deletions(-) rename src/clib/{dynamic_api.c => dynamic_api.cpp} (79%) diff --git a/.clang-format-ignore b/.clang-format-ignore index 0072faee9..53330b877 100644 --- a/.clang-format-ignore +++ b/.clang-format-ignore @@ -21,7 +21,7 @@ src/clib/cie_thin_cooling_rate_tables.h src/clib/cool1d_multi_g-cpp.C src/clib/cool1d_multi_g-cpp.h src/clib/cool_multi_time_g.cpp -src/clib/dynamic_api.c +src/clib/dynamic_api.cpp src/clib/fortran_func_decls.h src/clib/fortran_func_wrappers.hpp src/clib/grackle_macros.h diff --git a/src/clib/CMakeLists.txt b/src/clib/CMakeLists.txt index 693d6ea37..56a13215e 100644 --- a/src/clib/CMakeLists.txt +++ b/src/clib/CMakeLists.txt @@ -82,7 +82,6 @@ configure_file(auto_general.c.in auto_general.c @ONLY) add_library(Grackle_Grackle # C source files - dynamic_api.c grackle_units.c index_helper.c initialize_cloudy_data.c initialize_cloudy_data.h @@ -112,6 +111,7 @@ add_library(Grackle_Grackle cool_multi_time_g.cpp cool_multi_time_g.h dust_props.hpp dust/grain_species_info.cpp dust/grain_species_info.hpp + dynamic_api.cpp init_misc_species_cool_rates.cpp init_misc_species_cool_rates.hpp initialize_chemistry_data.cpp initialize_dust_yields.cpp initialize_dust_yields.hpp diff --git a/src/clib/dynamic_api.c b/src/clib/dynamic_api.cpp similarity index 79% rename from src/clib/dynamic_api.c rename to src/clib/dynamic_api.cpp index 27a9df915..e4beb2430 100644 --- a/src/clib/dynamic_api.c +++ b/src/clib/dynamic_api.cpp @@ -16,11 +16,13 @@ / software. ************************************************************************/ -#include -#include +#include +#include #include "grackle_chemistry_data.h" #include "status_reporting.h" +namespace { // stuff in an anonymous namespace is only visible to this file + /// This is collection of enumerators, with an enumerator named for EVERY /// parameter. Importantly, each enumerator has a unique value (that we use /// in a giant switch-statement). @@ -35,12 +37,15 @@ enum ParamEnum { // initialize int_param_l_, double_param_l_ & string_param_l_. They are sized // lists that respectively hold entries for each int, double, and string field // in chemistry_data. These are only used internally -typedef struct { +struct param_entry { const enum ParamEnum enum_val; const char * name; -} param_entry; +}; -typedef struct { const size_t len; const param_entry * entries; } param_list; +struct param_list { + const std::size_t len; + const param_entry * entries; +}; #define INIT_PAR_LIST(ENTRIES) {sizeof(ENTRIES) / sizeof(param_entry), ENTRIES} @@ -56,26 +61,27 @@ typedef struct { const size_t len; const param_entry * entries; } param_list; #define LIST_STRING_DOUBLE(FIELD) /* ... */ #define LIST_STRING_STRING(FIELD) {ParamEnum_ ## FIELD, #FIELD}, -static const param_entry int_param_entries_[] = { +constexpr param_entry int_param_entries_[] = { #define ENTRY(FIELD, TYPE, DEFAULT_VAL) LIST_INT_ ## TYPE(FIELD) #include "grackle_chemistry_data_fields.def" #undef ENTRY }; -static const param_list int_param_l_ = INIT_PAR_LIST(int_param_entries_); +constexpr param_list int_param_l_ = INIT_PAR_LIST(int_param_entries_); -static const param_entry double_param_entries_[] = { +constexpr param_entry double_param_entries_[] = { #define ENTRY(FIELD, TYPE, DEFAULT_VAL) LIST_DOUBLE_ ## TYPE(FIELD) #include "grackle_chemistry_data_fields.def" #undef ENTRY }; -static const param_list double_param_l_ = INIT_PAR_LIST(double_param_entries_); +constexpr param_list double_param_l_ = + INIT_PAR_LIST(double_param_entries_); -static const param_entry string_param_entries_[] = { +constexpr param_entry string_param_entries_[] = { #define ENTRY(FIELD, TYPE, DEFAULT_VAL) LIST_STRING_ ## TYPE(FIELD) #include "grackle_chemistry_data_fields.def" #undef ENTRY }; -static const param_list string_param_l_ = INIT_PAR_LIST(string_param_entries_); +constexpr param_list string_param_l_ = INIT_PAR_LIST(string_param_entries_); // define functions for accessing field values @@ -93,18 +99,18 @@ static const param_list string_param_l_ = INIT_PAR_LIST(string_param_entries_); /// retrieves a pointer to the field of my_chemistry that's named ``name``. /// -/// This returns a NULL pointer if: my_chemistry is NULL, the field doesn't +/// This returns a nullptr pointer if: my_chemistry is nullptr, the field doesn't /// exist, or the field doesn't have the specified type static void* get_field_ptr_(chemistry_data* my_chemistry, const char* name, const param_list my_param_l) { - if (my_chemistry == NULL) { return NULL; } + if (my_chemistry == nullptr) { return nullptr; } // lookup the enum value associated with name enum ParamEnum enum_val = ParamEnum_NUM_ENTRIES; - for (size_t param_index = 0; param_index < my_param_l.len; param_index++){ + for (std::size_t param_index = 0; param_index < my_param_l.len; param_index++){ const param_entry* entry = my_param_l.entries + param_index; - if (strcmp(entry->name, name) == 0) { + if (std::strcmp(entry->name, name) == 0) { enum_val = entry->enum_val; break; } @@ -117,11 +123,15 @@ static void* get_field_ptr_(chemistry_data* my_chemistry, const char* name, #include "grackle_chemistry_data_fields.def" #undef ENTRY - case ParamEnum_NUM_ENTRIES: return NULL; // <- unknown name + case ParamEnum_NUM_ENTRIES: return nullptr; // <- unknown name } GR_INTERNAL_UNREACHABLE_ERROR(); } +} // anonymous namespace + +extern "C" { + int* local_chemistry_data_access_int(chemistry_data* my_chemistry, const char* param_name) { return (int*)get_field_ptr_(my_chemistry, param_name, int_param_l_); } @@ -134,6 +144,8 @@ char** local_chemistry_data_access_string(chemistry_data* my_chemistry, const char* param_name) { return (char**)get_field_ptr_(my_chemistry, param_name, string_param_l_); } +} // extern "C" + // define functions for accessing the names of chemistry_data: // param_name_int, param_name_double, param_name_string // @@ -141,9 +153,11 @@ char** local_chemistry_data_access_string(chemistry_data* my_chemistry, // serialization. // returns the name of the ``i``th parameter of the specified type. This returns -// NULL when there are ``i`` or fewer parameters of the specified type -static const char* param_name_(size_t i, const param_list my_param_list) -{ return (i < my_param_list.len) ? my_param_list.entries[i].name : NULL; } +// nullptr when there are ``i`` or fewer parameters of the specified type +static const char* param_name_(std::size_t i, const param_list my_param_list) +{ return (i < my_param_list.len) ? my_param_list.entries[i].name : nullptr; } + +extern "C" { const char* param_name_int(unsigned int i) {return param_name_(i,int_param_l_);} @@ -154,14 +168,16 @@ const char* param_name_string(unsigned int i) // function to query the number of parameters of chemistry_data of a given type unsigned int grackle_num_params(const char* type_name){ - if (type_name == NULL) { + if (type_name == nullptr) { return 0; - } else if (strcmp(type_name, "int") == 0) { + } else if (std::strcmp(type_name, "int") == 0) { return (unsigned int) int_param_l_.len; - } else if (strcmp(type_name, "double") == 0) { + } else if (std::strcmp(type_name, "double") == 0) { return (unsigned int) double_param_l_.len; - } else if (strcmp(type_name, "string") == 0) { + } else if (std::strcmp(type_name, "string") == 0) { return (unsigned int) string_param_l_.len; } return 0; } + +} // extern "C" From 8bbb18b6f6af2044e17522a5ae9e04a31f440a82 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 21:20:21 -0500 Subject: [PATCH 18/19] shift to using a scoped enum --- src/clib/dynamic_api.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/clib/dynamic_api.cpp b/src/clib/dynamic_api.cpp index e4beb2430..948b2964c 100644 --- a/src/clib/dynamic_api.cpp +++ b/src/clib/dynamic_api.cpp @@ -26,19 +26,19 @@ namespace { // stuff in an anonymous namespace is only visible to this file /// This is collection of enumerators, with an enumerator named for EVERY /// parameter. Importantly, each enumerator has a unique value (that we use /// in a giant switch-statement). -enum ParamEnum { -#define ENTRY(FIELD, TYPE, DEFAULT_VAL) ParamEnum_ ## FIELD, +enum class ParamEnum { +#define ENTRY(FIELD, TYPE, DEFAULT_VAL) FIELD, #include "grackle_chemistry_data_fields.def" #undef ENTRY - ParamEnum_NUM_ENTRIES // <- always last (so it specifies number of entries) + NUM_ENTRIES // <- always last (so it specifies number of entries) }; // initialize int_param_l_, double_param_l_ & string_param_l_. They are sized // lists that respectively hold entries for each int, double, and string field // in chemistry_data. These are only used internally struct param_entry { - const enum ParamEnum enum_val; + const ParamEnum enum_val; const char * name; }; @@ -49,17 +49,17 @@ struct param_list { #define INIT_PAR_LIST(ENTRIES) {sizeof(ENTRIES) / sizeof(param_entry), ENTRIES} -#define LIST_INT_INT(FIELD) {ParamEnum_ ## FIELD, #FIELD}, +#define LIST_INT_INT(FIELD) {ParamEnum::FIELD, #FIELD}, #define LIST_INT_DOUBLE(FIELD) /* ... */ #define LIST_INT_STRING(FIELD) /* ... */ #define LIST_DOUBLE_INT(FIELD) /* ... */ -#define LIST_DOUBLE_DOUBLE(FIELD) {ParamEnum_ ## FIELD, #FIELD}, +#define LIST_DOUBLE_DOUBLE(FIELD) {ParamEnum::FIELD, #FIELD}, #define LIST_DOUBLE_STRING(FIELD) /* ... */ #define LIST_STRING_INT(FIELD) /* ... */ #define LIST_STRING_DOUBLE(FIELD) /* ... */ -#define LIST_STRING_STRING(FIELD) {ParamEnum_ ## FIELD, #FIELD}, +#define LIST_STRING_STRING(FIELD) {ParamEnum::FIELD, #FIELD}, constexpr param_entry int_param_entries_[] = { #define ENTRY(FIELD, TYPE, DEFAULT_VAL) LIST_INT_ ## TYPE(FIELD) @@ -107,7 +107,7 @@ static void* get_field_ptr_(chemistry_data* my_chemistry, const char* name, if (my_chemistry == nullptr) { return nullptr; } // lookup the enum value associated with name - enum ParamEnum enum_val = ParamEnum_NUM_ENTRIES; + enum ParamEnum enum_val = ParamEnum::NUM_ENTRIES; for (std::size_t param_index = 0; param_index < my_param_l.len; param_index++){ const param_entry* entry = my_param_l.entries + param_index; if (std::strcmp(entry->name, name) == 0) { @@ -119,11 +119,11 @@ static void* get_field_ptr_(chemistry_data* my_chemistry, const char* name, // now use a switch statement to actually return the appropriate pointer switch (enum_val) { #define ENTRY(FIELD, TYPE, DEFAULT_VAL) \ - case ParamEnum_ ## FIELD: { return (void*)(&my_chemistry->FIELD); } + case ParamEnum::FIELD: { return (void*)(&my_chemistry->FIELD); } #include "grackle_chemistry_data_fields.def" #undef ENTRY - case ParamEnum_NUM_ENTRIES: return nullptr; // <- unknown name + case ParamEnum::NUM_ENTRIES: return nullptr; // <- unknown name } GR_INTERNAL_UNREACHABLE_ERROR(); } From f8916fc1e4ee884c0b75cc3edd1b7c5c33166f02 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 21:26:34 -0500 Subject: [PATCH 19/19] tweak a few comments --- src/clib/dynamic_api.cpp | 13 ++++++++----- src/clib/rate_utils.cpp | 21 --------------------- 2 files changed, 8 insertions(+), 26 deletions(-) diff --git a/src/clib/dynamic_api.cpp b/src/clib/dynamic_api.cpp index 948b2964c..325e1ce43 100644 --- a/src/clib/dynamic_api.cpp +++ b/src/clib/dynamic_api.cpp @@ -99,8 +99,10 @@ constexpr param_list string_param_l_ = INIT_PAR_LIST(string_param_entries_); /// retrieves a pointer to the field of my_chemistry that's named ``name``. /// -/// This returns a nullptr pointer if: my_chemistry is nullptr, the field doesn't -/// exist, or the field doesn't have the specified type +/// This returns a nullptr pointer if: +/// - my_chemistry is nullptr +/// - the field doesn't exist +/// - the field doesn't have the specified type static void* get_field_ptr_(chemistry_data* my_chemistry, const char* name, const param_list my_param_l) { @@ -152,8 +154,9 @@ char** local_chemistry_data_access_string(chemistry_data* my_chemistry, // These are primarily needed for testing purposes and can be used for // serialization. -// returns the name of the ``i``th parameter of the specified type. This returns -// nullptr when there are ``i`` or fewer parameters of the specified type +/// returns the name of the ``i``th parameter of the specified type. This +/// returns nullptr when there are ``i`` or fewer parameters of the specified +/// type static const char* param_name_(std::size_t i, const param_list my_param_list) { return (i < my_param_list.len) ? my_param_list.entries[i].name : nullptr; } @@ -180,4 +183,4 @@ unsigned int grackle_num_params(const char* type_name){ return 0; } -} // extern "C" +} // extern "C" diff --git a/src/clib/rate_utils.cpp b/src/clib/rate_utils.cpp index f24cf66fd..ec2e87589 100644 --- a/src/clib/rate_utils.cpp +++ b/src/clib/rate_utils.cpp @@ -18,27 +18,6 @@ #include "LUT.hpp" // CollisionalRxnLUT #include "opaque_storage.hpp" // gr_opaque_storage -// In comparison to the dynamic API for accessing elements of chemistry_data, -// we have explicitly opted NOT to make use of offsetof to access arbitrary -// values within a struct. -// -> while `offsetof` may lead to somewhat simpler code, the validity of using -// it with pointer-arithmetic to access members of structs is murky at best. -// - Things become murky when you consider that the C standard models an -// abstract machine and doesn't place the strongest (or clearest) -// constraints on a compiler when you start arbitrarily messing with -// memory -// - C's object model (and maybe also pointer provenance) is relevant -// -> An extended discussion can be found in this stackoverflow answer and in -// the comments about the answer (https://stackoverflow.com/a/69936600). -// - the most compelling point is that it would be useless for the standard -// to define the `offsetof` if these semantics didn't work -// -> In any case, I'm much more skeptical that valid C++ code can make use of -// offsetof in this fashion (the object model is stricter and C++ provides a -// other machinery to accomplish similar things). Since we want to support -// compilation with a C++ compiler, this is the most important point -// offsetof in this fashion -// - // we have reserved the right to change this value at any time enum { UNDEFINED_RATE_ID_ = 0 };