Fable review of Boost.Histogram

[henryiii]

I did a sweep with Fable for issues. It independency found two existing issues that already have pending fixes, along with some others. My intent is to slowly fix these a few at a time, unless you'd rather something else. Is there anything you'd like me to prioritize or skip?

🤖 AI text below 🤖

All verification is done. Here's the consolidated review.

Boost.Histogram project review

Seven parallel review agents swept the codebase (core, axes, detail, accumulators, algorithms/fill, performance, modernization); I verified every finding below either by reproduction (ASan/UBSan/compiled repros, sources in the session scratchpad) or by reading the code myself. Two rediscovered known issues are excluded: #426 (broadcast-scalar fill wipes a chunk when a non-inclusive axis drops the first entry — found independently by two agents) and #430 (rank-0 indexed()).

Bugs — memory safety (all reproduced under ASan/UBSan)

1. multi_index.hpp:94 — stack-buffer overflow in dynamic-rank multi_index. The static-size specialization (line 50) throws std::invalid_argument on a size mismatch; the dynamic specialization copies into data_[32] with no check. h.at(std::vector<int>(40, 0)) on a dynamic-axes histogram overflows the stack (ASan-confirmed) instead of throwing.

2. detail/reduce_command.hpp:50 — stack-buffer overflow in reduce with too many positional commands. The bounds check only guards commands with an explicit axis index. reduce(h, rebin(2), rebin(2)) on a rank-1 tuple-axes histogram writes past the stack command buffer (ASan-confirmed); the explicit form rebin(1, 2) correctly throws.

3. storage_adaptor.hpp:85 — buffer overflow in array_impl converting constructor. operator= and reset both throw std::length_error when the source exceeds max_size(); the converting constructor doesn't check. Reachable purely through the public API by converting a histogram with more cells than the target std::array storage holds (ASan-confirmed).

Bugs — wrong results (reproduced)

4. fix: swapped modf outputs in circular variable::value() #438 axis/variable.hpp:214 — std::modf outputs swapped in circular value(). modf returns the fraction and stores the integral part in z, but the code uses the return value as bin index k and z as interpolation weight. Every fractional index and every integer index ≥ 2 on a non-equidistant circular variable axis returns a wrong edge/center (e.g. size-3 axis {0,1,3,4}: value(2) returns 2, should be 3). The existing test only uses a size-2 axis where the error cancels. Related to but distinct from Improve numerical accuracy of circular axis #380.

5. axis/regular.hpp:312 — circular axis puts finite values in the overflow bin. For a value an ulp below a period boundary, z -= std::floor(z) rounds to exactly 1.0 and the index becomes size(). axis::circular<>{4, 0, 1}.index(-1e-17) returns 4; the count silently lands in overflow (or is dropped without the overflow bin).

6. unlimited_storage.hpp:347 — cell self-assignment zeroes the value. reference::operator= zeroes the destination before reading through the visited source pointer, so h.at(0) = h.at(0) sets the cell to 0. The comment claims self-assignment safety but only covers reallocation, not aliasing.

7. Float→int conversion UB on extreme finite inputs (UBSan-confirmed): axis/regular.hpp:340,351 (update() casts 1e300 * size() to int), axis/integer.hpp:109,115 (x - min_ signed overflow for index(INT_MAX) with negative min_; also misbins as underflow on wrap), axis/integer.hpp:133 (cast of large finite double to long in update()). All reachable from a plain fill on growth axes; the project's own histogram_ubasan CI variant would flag these given a test input.

8. utility/binomial_proportion_interval.hpp:123 — std::terminate at ≥ ~8.3σ. The noexcept conversion deviation → confidence_level computes a cl that rounds to exactly 1.0, and the confidence_level ctor throws inside noexcept → terminate (reproduced). Same pattern in the noexcept operator* with a negative factor.

Bugs — lower severity

9. ostream.hpp:84 — tabular_ostream_wrapper swaps in its own rdbuf and restores it only in complete(); a throwing user operator<< leaves the caller's stream pointing at a destroyed streambuf (use-after-scope on the next write). Needs a restoring destructor/RAII guard.
10. storage_adaptor.hpp:195 — map-backed operator*= skips absent cells, so h *= NaN/inf leaves untouched cells at 0 while dense storage yields NaN everywhere; the sibling operator/= handles exactly this case.
11. detail/accumulator_traits.hpp:59-65 — two overloads at the same priority<2> are ambiguous for an accumulator with both operator() and operator+=(weight_type); hard compile error (confirmed by compilation).
12. detail/chunk_vector.hpp:79-83 — insert() has an inverted size check (throws on the valid multiple-of-chunk case) and doesn't even compile when reached via collector::operator+=; experimental type, only test coverage never calls it.
13. detail/term_info.hpp:80 — width() returns min(col, w) when the ioctl succeeded but COLUMNS is unset (col == 0), discarding the detected width → garbled terminal plots below 78 columns.
14. detail/static_vector.hpp:54,59 — at() is noexcept yet throws std::out_of_range → terminate; currently latent (no out-of-range caller).
15. detail/priority.hpp:10 — uses std::size_t with only <cstdint> included; fails standalone on libc++ (public headers unaffected).

Performance

• perf: hoist loop-invariant division out of reduce's cell loop #439 algorithm/reduce.hpp:421-425 — the per-cell loop recomputes the loop-invariant (end - begin) / merge division for every axis of every cell and divides by merge even when it's 1; hoisting into the precomputed opts buffer removes O(ncells × rank) integer divisions. Verified in code; the clearest win.
• histogram.hpp:453 (and -=, *=, /= siblings) — histogram-histogram arithmetic with unlimited_storage does two buffer-type dispatches per cell; a storage-level visit-once loop (like the existing scalar *= path at line 562) would make the common merge-after-parallel-fill pattern vectorizable.
• algorithm/sum.hpp:55 — same per-cell dispatch through const_reference::operator double(); a single typed visit would remove it.
• (Henry: tested this, not measurable) detail/fill_n.hpp:139 — std::vector<axis::variant<...>> histograms always take the growth path if the variant merely lists a growing axis type (acknowledged by the TODO there); a cheap runtime "does any held axis actually grow" check like the existing all_inclusive check would skip the per-element shift bookkeeping.
• indexed.hpp:296 — dynamic-axes iterators embed a 32-entry index_data array (~1 KB each); iterating many small histograms pays multi-KB copies per indexed() call.

Simplification / modernization (all grep-verified)

Dead code in detail/detect.hpp: the void_t alias (line 31), detect_base::val(), has_method_push_back (69), requires_storage (184), requires_sequence_of_axis / requires_sequence_of_axis_variant (204-208) are unreferenced; is_indexable (71) and is_complete (129) are used only by their own unit test. The ignore_deprecation_warning_{begin,end}.hpp pair is included only by the ODR test with no deprecation markers anywhere to suppress, and detail/debug.hpp has zero includes (likely intentional dev tool). Optional: the detect macros could collapse onto mp11::mp_valid, and option.hpp's (B + 0) gcc-6 workaround is obsolete for every CI-tested compiler — though the README still claims gcc ≥ 5.5 support, so that one is a policy call.

The rejected-candidates lists (things examined and cleared — Neumaier/Welford math, large_int arithmetic, safe_comparison, interval formulas against Brown/Cai/DasGupta, linearize/growth paths, etc.) and all repro programs are in the scratchpad if you want to look. /henry-generic-review-followup can pick these up; I'd start with the three ASan overflows (items 1–3) and the two circular-axis result bugs (4–5), each of which needs a regression test first per your workflow.