From c9bf62ee7cb5c9de0b9bcd1308a6bd83150bc9ee Mon Sep 17 00:00:00 2001 From: Thomas Date: Mon, 27 Jul 2026 08:08:45 +0200 Subject: [PATCH] refactor: drop the runtime size from statically sized scalars m_size only means anything when TSize is Dynamic. For every other variant it was eight bytes that nothing reads, and the constructor taking Data never set them, which is what clang-tidy reported in #80: hyperjet.h:322: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject] 322 | DDScalar(const Data &data) : m_data(data) { 56 | index m_size; <- uninitialized Nothing read the field, but every copy of such an object copied an indeterminate value, which is UB to read and would trip MSan. The field now has an empty stand-in type for the static variants, marked [[no_unique_address]], so a static scalar is exactly its data: before after DDScalar<1, double, 3> 40 32 DDScalar<2, double, 3> 88 80 DDScalar<2, double, 8> 368 360 DDScalar<2, double, 16> 1232 1224 DDScalar<2, double, Dynamic> 32 32 The initializer list constructor is the one place that ran for both variants and set the field from its member initializer list, so it computes the size into a local and only stores it when there is somewhere to store it. This also settles the layout question a NumPy dtype would ask of the element type: a static scalar is now payload-sized and trivially copyable, and the dynamic one is neither, which is why only the static variants could ever back a dtype. Not a performance change, but measured rather than assumed. Five alternating runs of `a * b + a.sin()` over 20k elements, -O3 -DNDEBUG: size 3 unchanged at about 10.85 ns per element, size 8 consistently a little faster, 51.6-52.4 against 52.0-52.9. The first measurement suggested a 12 percent regression and was cold-start noise. C++ 102 test cases, 1310 assertions, in Release with -Werror and under clang with ASan+UBSan Python 172 passed, exported API identical against a build of the previous state clang-tidy down to 50 findings, the uninitialized field gone --- include/hyperjet/hyperjet.h | 18 ++++++++++++++---- test/src/test_variants.cpp | 22 ++++++++++++++++++++-- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/include/hyperjet/hyperjet.h b/include/hyperjet/hyperjet.h index 3c3931e..ee009cd 100644 --- a/include/hyperjet/hyperjet.h +++ b/include/hyperjet/hyperjet.h @@ -18,8 +18,10 @@ namespace hyperjet { #if defined(_MSC_VER) #define HYPERJET_INLINE __forceinline +#define HYPERJET_NO_UNIQUE_ADDRESS [[msvc::no_unique_address]] #else #define HYPERJET_INLINE __attribute__((always_inline)) inline +#define HYPERJET_NO_UNIQUE_ADDRESS [[no_unique_address]] #endif using index = std::ptrdiff_t; @@ -47,13 +49,19 @@ class DDScalar { using DynamicStorage = std::vector; using StaticStorage = std::array; + // A statically sized scalar knows its size from TSize, so it carries no + // field for it. The empty stand-in keeps m_size spellable in both variants. + struct NoSize {}; + public: using Type = DDScalar; using Scalar = TScalar; using Data = typename std::conditional::type; - index m_size; + using SizeStorage = std::conditional_t; + + HYPERJET_NO_UNIQUE_ADDRESS SizeStorage m_size; Data m_data; public: @@ -331,13 +339,15 @@ class DDScalar { static_assert(is_dynamic()); } - DDScalar(std::initializer_list data) - : m_size(size_from_data_length(length(data))) { + DDScalar(std::initializer_list data) { static_assert(0 < order() && order() <= 2); - check_valid_size(m_size); + const index s = size_from_data_length(length(data)); + + check_valid_size(s); if constexpr (is_dynamic()) { + m_size = s; m_data.assign(data.begin(), data.end()); } else { std::copy(data.begin(), data.end(), m_data.begin()); diff --git a/test/src/test_variants.cpp b/test/src/test_variants.cpp index f5e5a0a..e4b0804 100644 --- a/test/src/test_variants.cpp +++ b/test/src/test_variants.cpp @@ -4,8 +4,9 @@ #include -#include // array -#include // vector +#include // array +#include // is_trivially_copyable +#include // vector // Coverage across all four combinations of order and sizing. // @@ -106,6 +107,23 @@ void check(const T &actual, const std::vector &expected) { } } +// A statically sized scalar is exactly its data. The runtime size is only +// meaningful for the dynamic variant, where it also rules out trivial copying. +// Both properties are what an element type has to offer a NumPy dtype. + +TEST_CASE("Static scalars are exactly their data") { + CHECK(sizeof(DDScalar<1, double, 0>) == sizeof(DDScalar<1, double, 0>::Data)); + CHECK(sizeof(DDScalar<1, double, 3>) == sizeof(DDScalar<1, double, 3>::Data)); + CHECK(sizeof(DDScalar<2, double, 3>) == sizeof(DDScalar<2, double, 3>::Data)); + CHECK(sizeof(DDScalar<2, double, 8>) == sizeof(DDScalar<2, double, 8>::Data)); + + CHECK(std::is_trivially_copyable_v>); + CHECK(std::is_trivially_copyable_v>); + CHECK(std::is_standard_layout_v>); + + CHECK_FALSE(std::is_trivially_copyable_v>); +} + TEST_CASE_TEMPLATE("variants: values", T, D1, X1, D2, X2) { auto a = make(A);