From 13d0f4c927ec9546b571c865fa064b34998ba829 Mon Sep 17 00:00:00 2001 From: Kyle Knoepfel Date: Fri, 21 Aug 2026 11:52:21 -0500 Subject: [PATCH 1/3] refactor(core): adopt gsl::not_null where appropriate --- CMakeLists.txt | 1 + phlex/driver.hpp | 13 +++++++------ phlex/model/CMakeLists.txt | 1 + phlex/model/fixed_hierarchy.hpp | 6 ++++-- phlex/model/handle.hpp | 10 +++++++--- phlex/model/products.cpp | 2 +- phlex/model/products.hpp | 10 +++++++--- 7 files changed, 28 insertions(+), 15 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c71fc9af1..4512c7142 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -163,6 +163,7 @@ find_package(Boost REQUIRED COMPONENTS json EXPORT) find_package(TBB REQUIRED EXPORT) find_package(fmt REQUIRED EXPORT) find_package(spdlog REQUIRED EXPORT) +find_package(Microsoft.GSL REQUIRED EXPORT) # Dependencies required by internal implementation find_package(Boost REQUIRED COMPONENTS program_options) diff --git a/phlex/driver.hpp b/phlex/driver.hpp index 64c9d8c33..2e15673ae 100644 --- a/phlex/driver.hpp +++ b/phlex/driver.hpp @@ -12,7 +12,8 @@ #include "boost/mp11/algorithm.hpp" #include "fmt/format.h" -#include +#include + #include #include #include @@ -35,13 +36,13 @@ namespace phlex::detail { using driver_shim_t = void(driver_proxy const&, configuration const&, driver_bundle*); template - std::remove_cvref_t const& as_driver_source(source const* src, std::size_t index) + std::remove_cvref_t const& as_driver_source(gsl::not_null src, + std::size_t index) { - assert(src != nullptr); - using expected_source_t = std::remove_cvref_t; - if (auto const* casted = dynamic_cast(src)) { + auto const* src_ptr = src.get(); + if (auto const* casted = dynamic_cast(src_ptr)) { return *casted; } @@ -49,7 +50,7 @@ namespace phlex::detail { fmt::format("Driver source type mismatch at source index {}: expected '{}' but got '{}'.", index, boost::core::demangle(typeid(expected_source_t).name()), - boost::core::demangle(typeid(*src).name()))); + boost::core::demangle(typeid(*src_ptr).name()))); } template diff --git a/phlex/model/CMakeLists.txt b/phlex/model/CMakeLists.txt index e9eb1ad5f..76d1fd776 100644 --- a/phlex/model/CMakeLists.txt +++ b/phlex/model/CMakeLists.txt @@ -20,6 +20,7 @@ cet_make_library( Boost::boost spdlog::spdlog fmt::fmt + Microsoft.GSL::GSL PRIVATE phlex::utilities TBB::tbb diff --git a/phlex/model/fixed_hierarchy.hpp b/phlex/model/fixed_hierarchy.hpp index 805d773be..24b6b8f47 100644 --- a/phlex/model/fixed_hierarchy.hpp +++ b/phlex/model/fixed_hierarchy.hpp @@ -6,6 +6,8 @@ #include "phlex/model/fwd.hpp" #include "phlex/model/layer_path.hpp" +#include + #include #include #include @@ -37,8 +39,8 @@ namespace phlex { data_cell_index_ptr index_; // Non-owning pointers to the enclosing hierarchy and driver; data_cell_cursor is a // short-lived, freely copyable view and does not manage their lifetimes. - fixed_hierarchy const* hierarchy_; - detail::framework_driver* driver_; + gsl::not_null hierarchy_; + gsl::not_null driver_; }; /// @brief Callable object that yields data cells to the framework driver. diff --git a/phlex/model/handle.hpp b/phlex/model/handle.hpp index b807b54a0..551950cf0 100644 --- a/phlex/model/handle.hpp +++ b/phlex/model/handle.hpp @@ -5,6 +5,8 @@ #include "phlex/model/fwd.hpp" #include "phlex/model/product_specification.hpp" +#include + #include #include #include @@ -83,7 +85,7 @@ namespace phlex { handle(handle&&) noexcept = default; handle& operator=(handle&&) noexcept = default; - const_pointer operator->() const noexcept { return product_; } + const_pointer operator->() const noexcept { return product_.get(); } [[nodiscard]] const_reference operator*() const noexcept { return *operator->(); } // NOLINTBEGIN(google-explicit-constructor) - Implicit conversion is intentional operator const_reference() const noexcept { return operator*(); } @@ -116,8 +118,10 @@ namespace phlex { } private: - const_pointer product_; // Non-null, by construction - class data_cell_index const* id_; // Non-null, by construction + // Non-owning pointers to the product and its data-cell index; both are non-null by + // construction, as enforced by gsl::not_null. + gsl::not_null product_; + gsl::not_null id_; experimental::identifier creator_plugin_; experimental::identifier creator_algorithm_; experimental::identifier suffix_; diff --git a/phlex/model/products.cpp b/phlex/model/products.cpp index 12953f894..b10ba158e 100644 --- a/phlex/model/products.cpp +++ b/phlex/model/products.cpp @@ -17,7 +17,7 @@ namespace phlex::detail { products::size_type products::size() const noexcept { return products_.size(); } bool products::empty() const noexcept { return products_.empty(); } - product_base const* products::find_product(product_specification const& spec) const + gsl::not_null products::find_product(product_specification const& spec) const { auto it = std::ranges::find(products_, spec, [](auto const& p) -> auto const& { return p.first; }); diff --git a/phlex/model/products.hpp b/phlex/model/products.hpp index 2732a09f7..fbb7422e7 100644 --- a/phlex/model/products.hpp +++ b/phlex/model/products.hpp @@ -5,6 +5,8 @@ #include "phlex/model/product_specification.hpp" +#include + #include #include #include @@ -87,9 +89,9 @@ namespace phlex::detail { template T const& get(product_specification const& spec) const { - auto const* available_product = find_product(spec); + auto const available_product = find_product(spec); - if (auto const* desired_product = dynamic_cast const*>(available_product)) { + if (auto const* desired_product = dynamic_cast const*>(available_product.get())) { return desired_product->obj; } @@ -102,7 +104,9 @@ namespace phlex::detail { bool empty() const noexcept; private: - product_base const* find_product(product_specification const& spec) const; + // Throws if no product matches the specification; the returned pointer is therefore + // never null. + gsl::not_null find_product(product_specification const& spec) const; static void throw_mismatched_type [[noreturn]] (product_specification const& spec, char const* requested_type, char const* available_type); From 8564997054ee813a4d1528f3c2b9c2090131ba42 Mon Sep 17 00:00:00 2001 From: Kyle Knoepfel Date: Tue, 25 Aug 2026 08:47:10 -0500 Subject: [PATCH 2/3] fix(cmake): declare GSL content as Microsoft.GSL FetchContent derives the redirect package config it writes to CMAKE_FIND_PACKAGE_REDIRECTS_DIR from the declared content name, not from FIND_PACKAGE_ARGS NAMES. Declaring the content as GSL therefore generated GSLConfig.cmake/gsl-config.cmake, while find_package(Microsoft.GSL REQUIRED EXPORT) searches for Microsoft.GSLConfig.cmake. Absent an installed Microsoft.GSL package the lookup failed, because upstream defaults GSL_INSTALL to PROJECT_IS_TOP_LEVEL and so installs no config when GSL is built as a nested project. Declare and make the content available as Microsoft.GSL so the generated redirect matches the package name used by the later lookup. NAMES is no longer needed, and the explicit find_package call retains cetmodules' EXPORT semantics, which record GSL as a transitive dependency in the installed package config. --- CMakeLists.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4512c7142..332669d09 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,12 +22,12 @@ FetchContent_Declare( ) # ... and Microsoft's C++ Guideline Support Library FetchContent_Declare( - GSL + Microsoft.GSL GIT_REPOSITORY https://github.com/microsoft/GSL GIT_TAG v4.2.0 GIT_SHALLOW ON EXCLUDE_FROM_ALL # Do not install - FIND_PACKAGE_ARGS NAMES Microsoft.GSL + FIND_PACKAGE_ARGS ) # ... and the Mimic C++ mocking framework FetchContent_Declare( @@ -90,7 +90,7 @@ set(DART_TESTING_TIMEOUT 90 CACHE STRING "Timeout (s) for Dart/CTest runs") set(CTEST_TEST_TIMEOUT 90 CACHE STRING "Per-test timeout (s) for CTest") # Make tools available -FetchContent_MakeAvailable(Catch2 GSL mimicpp) +FetchContent_MakeAvailable(Catch2 Microsoft.GSL mimicpp) # Avoid use of __COUNTER__ to prevent -pedantic warnings in Catch2's TEST_CASE macro set(CATCH_CONFIG_COUNTER OFF) From 256d1c3fab1cb56c66e3ab13d5314cd92be3c280 Mon Sep 17 00:00:00 2001 From: Kyle Knoepfel Date: Tue, 25 Aug 2026 09:08:22 -0500 Subject: [PATCH 3/3] fix(model): reject null product pointers before storage --- phlex/model/products.hpp | 6 +++++- test/CMakeLists.txt | 3 +++ test/products_test.cpp | 11 +++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 test/products_test.cpp diff --git a/phlex/model/products.hpp b/phlex/model/products.hpp index fbb7422e7..fd664686d 100644 --- a/phlex/model/products.hpp +++ b/phlex/model/products.hpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -48,7 +49,10 @@ namespace phlex::detail { product_ptr product_for(T&& t) { if constexpr (std::convertible_to) { - return std::forward(t); + if (auto product = product_ptr{std::forward(t)}) { + return product; + } + throw std::runtime_error("Cannot store a null product pointer."); } else { return std::make_unique>>(std::forward(t)); } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index bb5197188..1ac190098 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -191,6 +191,9 @@ cet_test(product_handle USE_CATCH2_MAIN SOURCE product_handle_test.cpp LIBRARIES cet_test(product_store USE_CATCH2_MAIN SOURCE product_store_test.cpp LIBRARIES phlex::core_internal ) +cet_test(products USE_CATCH2_MAIN SOURCE products_test.cpp LIBRARIES + phlex::model_internal +) cet_test( fold USE_CATCH2_MAIN diff --git a/test/products_test.cpp b/test/products_test.cpp new file mode 100644 index 000000000..ee9c7b34d --- /dev/null +++ b/test/products_test.cpp @@ -0,0 +1,11 @@ +#include "phlex/model/products.hpp" + +#include "catch2/catch_test_macros.hpp" +#include "catch2/matchers/catch_matchers.hpp" + +TEST_CASE("Reject null product pointers", "[data model]") +{ + using namespace phlex::detail; + CHECK_THROWS_WITH(product_for(product_ptr{}), "Cannot store a null product pointer."); + CHECK_THROWS_WITH(product_for(nullptr), "Cannot store a null product pointer."); +}