test: extend td_init coverage + guard td_free(NULL) (follow-up to #41)

src/tdigest.c

@@ -104,6 +104,26 @@ static inline uint64_t cap_from_compression(uint64_t compression) {
     return (UINT64_C(6) * compression) + UINT64_C(10);
 }
 
+// Validate `compression` and compute the node-array capacity (cap = 6*compression + 10)
+// entirely in 64-bit width, WITHOUT allocating. Factored out of td_init() so the
+// accepted/rejected boundary can be probed in a test without committing tens of GiB of
+// backing storage (at cap ~ INT_MAX the two 8-byte node arrays total ~34 GB / ~32 GiB).
+// Returns 0 and writes *capacity on success; returns 1 on rejection and leaves *capacity
+// untouched. Rejections: non-finite, <= 0, > INT_MAX, or a capacity that would overflow int
+// or a size_t element count for either node array.
+static inline int capacity_from_compression(double compression, size_t *capacity) {
+    if (!isfinite(compression) || compression <= 0 || compression > INT_MAX) {
+        return 1;
+    }
+    const uint64_t capacity64 = cap_from_compression((uint64_t)compression);
+    if (capacity64 > INT_MAX || capacity64 > SIZE_MAX / sizeof(double) ||
+        capacity64 > SIZE_MAX / sizeof(long long)) {
+        return 1;
+    }
+    *capacity = (size_t)capacity64;
+    return 0;
+}
+
 static inline bool should_td_compress(td_histogram_t *h) {
     return ((h->merged_nodes + h->unmerged_nodes) >= (h->cap - 1));
 }
@@ -154,17 +174,12 @@ void td_reset(td_histogram_t *h) {
 
 int td_init(double compression, td_histogram_t **result) {
 
-    // Compute capacity in an explicitly 64-bit type so 6 * compression + 10 cannot wrap at the
-    // width of int or size_t before it is validated. cap and the node indexes are stored as int.
-    if (!isfinite(compression) || compression <= 0 || compression > INT_MAX) {
-        return 1;
-    }
-    const uint64_t capacity64 = cap_from_compression((uint64_t)compression);
-    if (capacity64 > INT_MAX || capacity64 > SIZE_MAX / sizeof(double) ||
-        capacity64 > SIZE_MAX / sizeof(long long)) {
+    // Validate compression and size the node arrays in 64-bit width before narrowing to int
+    // (see capacity_from_compression). On rejection *result is left untouched.
+    size_t capacity;
+    if (capacity_from_compression(compression, &capacity) != 0) {
         return 1;
     }
-    const size_t capacity = (size_t)capacity64;
     td_histogram_t *histogram;
     histogram = (td_histogram_t *)td_malloc_(sizeof(td_histogram_t));
     if (!histogram) {
@@ -197,6 +212,12 @@ td_histogram_t *td_new(double compression) {
 }
 
 void td_free(td_histogram_t *histogram) {
+    // NULL guard: td_new() returns NULL for invalid compression (non-finite / <= 0 /
+    // cap > INT_MAX) or allocation failure, so the idiomatic td_free(td_new(bad)) cleanup
+    // would otherwise dereference NULL. (td_new()'s validation landed in #41.)
+    if (!histogram) {
+        return;
+    }
     if (histogram->nodes_mean) {
         td_free_((void *)(histogram->nodes_mean));
     }

src/tdigest.h

@@ -62,7 +62,9 @@ extern "C" {
  * 1000 is extremely large.
  * The number of centroids retained will be a smallish (usually less than 10) multiple of this
  * number.
- * @return the histogram on success, NULL if allocation failed.
+ * @return the histogram on success, NULL if `compression` is invalid (non-finite, <= 0, or so
+ * large that the centroid capacity would overflow) or if allocation failed. Because NULL is a
+ * valid return, `td_free(td_new(...))` is a safe cleanup idiom (td_free() tolerates NULL).
  */
 td_histogram_t *td_new(double compression);
 
@@ -74,15 +76,19 @@ td_histogram_t *td_new(double compression);
  * 1000 is extremely large.
  * The number of centroids retained will be a smallish (usually less than 10) multiple of this
  * number.
- * @param result Output parameter to capture allocated histogram.
- * @return 0 on success, 1 if allocation failed.
+ * @param result Output parameter to capture allocated histogram. On success `*result` is set to
+ * the new histogram; on failure `*result` is left untouched (never written), so a caller may seed
+ * it with a sentinel to distinguish "not written" from NULL.
+ * @return 0 on success, 1 if `compression` is invalid (non-finite, <= 0, or so large that the
+ * centroid capacity would overflow) or if allocation failed.
  */
 int td_init(double compression, td_histogram_t **result);
 
 /**
  * Frees the memory associated with the t-digest.
  *
- * @param h The histogram you want to free.
+ * @param h The histogram you want to free. Passing NULL is allowed and is a no-op, so the
+ * `td_free(td_new(...))` idiom is safe even when td_new() returns NULL.
  */
 void td_free(td_histogram_t *h);
 

tests/CMakeLists.txt

@@ -23,6 +23,16 @@ if (BUILD_TESTS)
     target_link_libraries(td_test tdigest m)
     enable_testing()
     add_test(td_test td_test)
+
+    # Capacity-boundary regression: includes tdigest.c to drive the allocation-free
+    # capacity_from_compression() helper at the exact accepted/rejected boundary without
+    # allocating the ~34 GB (~32 GiB) that the largest accepted compression would need.
+    # Use CMAKE_CURRENT_LIST_DIR so the src path stays correct when t-digest-c is consumed
+    # via add_subdirectory() (CMAKE_SOURCE_DIR would point at the outermost project).
+    add_executable(td_capacity_test unit/td_capacity_test.c)
+    target_include_directories(td_capacity_test PRIVATE ${CMAKE_CURRENT_LIST_DIR}/../src)
+    target_link_libraries(td_capacity_test m)
+    add_test(td_capacity_test td_capacity_test)
 endif()
 
 

tests/unit/td_capacity_test.c

@@ -0,0 +1,86 @@
+/*
+ * Capacity-boundary regression for td_init (follow-up to #41 / #44).
+ *
+ * The accepted side of the capacity guard is the largest compression whose node capacity
+ * (cap = 6*compression + 10) still fits within every limit the helper enforces: INT_MAX and the
+ * per-array element counts SIZE_MAX/sizeof(double) and SIZE_MAX/sizeof(long long). On a 64-bit
+ * size_t target INT_MAX binds; on a 32-bit size_t target the SIZE_MAX/8 element limit is smaller
+ * and binds instead, so the boundary is derived from the minimum rather than assuming INT_MAX.
+ * Verifying it through td_new()/td_init() is impractical because at cap ~ INT_MAX the two 8-byte
+ * node arrays total ~34 GB (~32 GiB). This test instead includes the translation unit and drives
+ * the factored, allocation-free helper capacity_from_compression() directly, so the exact
+ * boundary (and one past it) is checked without committing tens of GiB.
+ */
+#include <limits.h>
+#include <math.h>
+#include <stdint.h>
+#include <stdio.h>
+
+#include "tdigest.c" /* brings in the static capacity_from_compression + cap_from_compression */
+
+static int failures = 0;
+
+#define CHECK(cond, msg)                                                                           \
+    do {                                                                                           \
+        if (!(cond)) {                                                                             \
+            fprintf(stderr, "FAIL: %s\n", (msg));                                                  \
+            failures++;                                                                            \
+        }                                                                                          \
+    } while (0)
+
+int main(void) {
+    /* The binding capacity limit is the minimum of every check the helper enforces:
+     * INT_MAX and the per-array element counts SIZE_MAX/sizeof(double|long long). On 64-bit
+     * size_t this is INT_MAX; on 32-bit size_t it is the (smaller) SIZE_MAX/8. */
+    uint64_t max_capacity = (uint64_t)INT_MAX;
+    if (SIZE_MAX / sizeof(double) < max_capacity) {
+        max_capacity = (uint64_t)(SIZE_MAX / sizeof(double));
+    }
+    if (SIZE_MAX / sizeof(long long) < max_capacity) {
+        max_capacity = (uint64_t)(SIZE_MAX / sizeof(long long));
+    }
+
+    /* The largest integer compression whose capacity still fits within max_capacity. */
+    const long long max_ok = (long long)((max_capacity - 10) / 6);
+
+    /* Sanity on the arithmetic: this capacity must be within the limit, the next one must not. */
+    const uint64_t cap_ok = cap_from_compression((uint64_t)max_ok);
+    const uint64_t cap_over = cap_from_compression((uint64_t)(max_ok + 1));
+    CHECK(cap_ok <= max_capacity, "boundary capacity should be within the binding limit");
+    CHECK(cap_over > max_capacity, "boundary+1 capacity should exceed the binding limit");
+
+    const size_t SENTINEL = (size_t)0xA5A5A5A5A5A5A5A5ULL;
+    size_t cap;
+
+    /* Accepted boundary: returns 0 and writes a capacity that matches the formula. */
+    cap = SENTINEL;
+    CHECK(capacity_from_compression((double)max_ok, &cap) == 0,
+          "largest in-range compression must be accepted");
+    CHECK(cap == (size_t)cap_ok, "accepted boundary capacity must match 6*c+10");
+
+    /* One past the boundary: rejected, and *capacity is left untouched. */
+    cap = SENTINEL;
+    CHECK(capacity_from_compression((double)(max_ok + 1), &cap) == 1,
+          "compression just past the boundary must be rejected");
+    CHECK(cap == SENTINEL, "rejected input must leave *capacity untouched");
+
+    /* Invalid inputs are rejected and never touch *capacity. */
+    const double bad[] = {NAN, INFINITY, -INFINITY, 0.0, -1.0, (double)INT_MAX};
+    for (unsigned i = 0; i < sizeof(bad) / sizeof(bad[0]); ++i) {
+        cap = SENTINEL;
+        CHECK(capacity_from_compression(bad[i], &cap) == 1, "invalid compression must be rejected");
+        CHECK(cap == SENTINEL, "rejected input must leave *capacity untouched");
+    }
+
+    /* A small valid compression still computes the documented capacity. */
+    cap = SENTINEL;
+    CHECK(capacity_from_compression(100.0, &cap) == 0, "compression 100 must be accepted");
+    CHECK(cap == 610, "cap(100) must be 6*100 + 10");
+
+    if (failures == 0) {
+        printf("OK: capacity boundary max_ok=%lld cap=%llu\n", max_ok, (unsigned long long)cap_ok);
+        return 0;
+    }
+    fprintf(stderr, "%d capacity check(s) failed\n", failures);
+    return 1;
+}

tests/unit/td_test.c

@@ -521,8 +521,9 @@
     mu_assert_long_eq(0, td_init(1000000, &t));
     td_free(t);
 
-    mu_assert_long_eq(0, td_init(100000000, &t));
-    td_free(t);
+    // The exact accepted/rejected capacity boundary is covered by td_capacity_test, which
+    // exercises the no-alloc capacity helper directly instead of committing the ~34 GB (~32 GiB)
+    // the largest accepted compression would allocate here.
 }
 
 MU_TEST(test_quantiles) {
@@ -594,9 +595,102 @@
     td_free(t);
 }
 
+// td_free must tolerate NULL: this PR makes td_new() return NULL for invalid
+// compression, so td_free(td_new(bad)) is a natural cleanup pattern.
+MU_TEST(test_td_free_null) {
+    td_free(NULL);         // must not crash
+    td_free(td_new(-1.0)); // td_new returns NULL for invalid compression
+    td_free(td_new(NAN));
+}
+
+// The td_new() convenience wrapper must reject the same inputs as td_init().
+MU_TEST(test_td_new_rejects_bad_compression) {
+    mu_assert(td_new(NAN) == NULL, "td_new(NaN) must return NULL");
+    mu_assert(td_new(INFINITY) == NULL, "td_new(INF) must return NULL");
+    mu_assert(td_new(-1) == NULL, "td_new(-1) must return NULL");
+    mu_assert(td_new(0) == NULL, "td_new(0) must return NULL");
+    mu_assert(td_new((double)(((INT_MAX - 10) / 6) + 1)) == NULL,
+              "td_new above the capacity boundary must return NULL");
+}
+
+// td_init must leave *result untouched on failure (stronger than "stays NULL":
+// a sentinel proves td_init never writes the out-param on a rejected input).
+MU_TEST(test_td_init_result_untouched_on_failure) {
+    td_histogram_t sentinel_obj;
+    td_histogram_t *const sentinel = &sentinel_obj;
+    td_histogram_t *t;
+    t = sentinel;
+    mu_assert_long_eq(1, td_init(NAN, &t));
+    mu_assert(t == sentinel, "NaN: *result must be left untouched");
+    t = sentinel;
+    mu_assert_long_eq(1, td_init(0, &t));
+    mu_assert(t == sentinel, "zero: *result must be left untouched");
+    t = sentinel;
+    mu_assert_long_eq(1, td_init((double)(((INT_MAX - 10) / 6) + 1), &t));
+    mu_assert(t == sentinel, "overflow: *result must be left untouched");
+}
+
+// Capacity formula (cap = 6*compression + 10) and determinism at safe sizes,
+// plus the current behavior for sub-1 / fractional compression (accepted, floored).
+MU_TEST(test_td_init_cap_and_determinism) {
+    td_histogram_t *a = NULL, *b = NULL;
+    mu_assert_long_eq(0, td_init(1, &a));
+    mu_assert_long_eq(16, a->cap); // 6*1 + 10
+    td_free(a);
+    a = NULL;
+    mu_assert_long_eq(0, td_init(2, &a));
+    mu_assert_long_eq(22, a->cap); // 6*2 + 10
+    td_free(a);
+    a = NULL;
+    // determinism: same compression -> same cap
+    mu_assert_long_eq(0, td_init(500, &a));
+    mu_assert_long_eq(0, td_init(500, &b));
+    mu_assert_long_eq(3010, a->cap); // 6*500 + 10
+    mu_assert_long_eq(a->cap, b->cap);
+    td_free(a);
+    td_free(b);
+    // Sub-1 / fractional compression is currently ACCEPTED and floored to 0,
+    // yielding cap 10 (6*0 + 10); td_compression() then reports (int)0.5 == 0.
+    // Pins today's behavior (see PR discussion on whether to reject compression < 1).
+    a = NULL;
+    mu_assert_long_eq(0, td_init(0.5, &a));
+    mu_assert_long_eq(10, a->cap);
+    mu_assert_int_eq(0, td_compression(a));
+    td_free(a);
+}
+
+// A large but valid digest (compression 100000 -> cap 600010, ~9.6 MB) must not
+// just allocate but actually work end to end.
+MU_TEST(test_td_init_large_success_is_usable) {
+    td_histogram_t *t = NULL;
+    mu_assert_long_eq(0, td_init(100000, &t));
+    mu_assert(t != NULL, "large valid compression should allocate");
+    mu_assert_long_eq(600010, t->cap); // 6*100000 + 10
+    for (int i = 1; i <= 10000; ++i) {
+        mu_assert(td_add(t, (double)i, 1) == 0, "Insertion");
+    }
+    mu_assert(td_compress(t) == 0, "compress large digest");
+    mu_assert_double_eq(1.0, td_min(t));
+    mu_assert_double_eq(10000.0, td_max(t));
+    mu_assert_long_eq(10000, td_size(t));
+    mu_assert(td_centroid_count(t) <= t->cap, "centroid count must stay within cap");
+    // Store the result and assert it is finite first: mu_assert_double_eq_epsilon does NOT fail
+    // for NaN (fabs(expected - NaN) is NaN, and NaN > epsilon is false), so the epsilon check
+    // alone would pass even if td_quantile() returned NaN.
+    const double median = td_quantile(t, 0.5);
+    mu_assert(isfinite(median), "median must be finite");
+    mu_assert_double_eq_epsilon(5000.5, median, 50.0);
+    td_free(t);
+}
+
 MU_TEST_SUITE(test_suite) {
     MU_RUN_TEST(test_basic);
     MU_RUN_TEST(test_td_init);
+    MU_RUN_TEST(test_td_free_null);
+    MU_RUN_TEST(test_td_new_rejects_bad_compression);
+    MU_RUN_TEST(test_td_init_result_untouched_on_failure);
+    MU_RUN_TEST(test_td_init_cap_and_determinism);
+    MU_RUN_TEST(test_td_init_large_success_is_usable);
     MU_RUN_TEST(test_compress_small);
     MU_RUN_TEST(test_compress_large);
     MU_RUN_TEST(test_nans);