Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
I have an issue with this function: /**
* @brief Determines if a signed integer value is a power of two.
*
* @tparam T Type of the input.
* @param n Value to check.
* @return True if the input is a power of two, false otherwise.
*
* @note Prefer ccm::ext::safe::is_power_of_two for signed inputs to avoid signed
* overflow on the minimum representable value.
*/
template <typename T, std::enable_if_t<std::is_signed_v<T> && !std::is_same_v<T, bool>, bool> = true>
constexpr bool is_power_of_two(T n) noexcept
{ return n && !(n & (n - T(1))); }inside is_power_of_two.hpp file. Another issue being that for functions |
Good catch! The original intention was for the unqualified signed overload to be the lean bit-twiddle version and for After some analysis though I compared the codegen for both and there is honestly no performance difference between them that I can find. They come out branchless at the same instruction count on clang, and gcc emits the same shape with a single compare, so guarding against the minimum value should costs nothing. Given that, there is no reason I can think of to keep a default that is wrong at the minimum value, so I am going to replace the unsafe version with the safe one. Thanks for flagging it! |
Also fair point! These angle helpers borrow ideas from Unity and GLSL so that is where the original idea came from, but I agree that taking the approach of mirroring the rest of CCMath and the Standard by using radians is the right call here. So I've decided we will key these off radians rather than add a _deg suffix. delta_angle, lerp_angle, and move_towards_angle now take and return radians, which keeps them in line with the trig functions. Also, while I was at it I decided to I flip the ext guard convention so the guarded behavior is now the default and the raw form lives under ccm::ext::unsafe rather than the opposite like before. I also went through the codegen on each unsafe variant and removed all the versions that did not produce better codegen. |
ProfessionalMenace
left a comment
There was a problem hiding this comment.
Found nothing major. Refactoring fixed a lot of the readability issues I had.
CCMath 0.3.0 Release Candidate
This is the review PR for v0.3.0. The goal is to catch any remaining blockers before the
release is cut.
It puts the entire v0.2.0 to v0.3.0 change set in one diff. It is review-only and will not be
merged. The base branch
review-base/v0.2.0is a frozen snapshot of thev0.2.0tag, so thediff is exactly what landed since the last release. The content itself already lives on
mainand on
release/0.3.x.How this review works: comments here are the review record. Fixes land as normal PRs into
main, after whichrelease/0.3.xis fast-forwarded. Record your approval as a review here.When review wraps up, the PR is closed and
review-base/v0.2.0is deleted. Treat it as a diffviewer and discussion thread.
The user-facing summary is in the release notes. What follows is for reviewers: what changed,
where the risk is, what is validated, and what would block the release.
What counts as blocking
Reasons to hold the release. Mark these clearly so they stand out from nits:
mode, on a supported path.
powlcall routed to the wrong kernel).extorfmanipfunctions (signature, NaN andinfinity behavior, constexpr-ness).
Most other items can be handled as nits or follow-ups unless they change the release risk.
Items under Known limitations are out of scope by design, not blockers, unless you can show
one breaks a supported path.
Where to focus review
These are the surfaces where a mistake means wrong math, so they earn the closest reading.
Validation has been run on most of them (see Validation status), but that lowers the
uncertainty rather than removing the need for attention.
Trigonometry (
doublesin,cos,tan)sin,cos, andtankernels.quadrant selection, sign-of-zero handling, and the inverse-trig edge cases (
asin,acos,atan,atan2). The earlier failure mode was large arguments falling outside thereduction, so large-input behavior is the thing to primarily probe.
Power (
doublepow,long doublepowl)powpath. The headline bug class fixed thisrelease was hi and lo halves stored swapped, which is harmless under round-to-nearest but
wrong in directed modes, so check each DD constant against its intended value and ordering.
powlkernel:powl_ld80_kernel.hpp
and its generated
powl_ld80_tables.hpp.
It compiles only where
long doubleis the 80-bit x87 extended format, so thedoubleandIEEE binary128
long doubleplatforms do not exercise it. This is preview quality (seeKnown limitations).
powpaths:pow_simd_impl.hppandpowf_simd_impl.hppin the same directory. Theserun only under runtime SIMD (not deterministic mode), so the check is that they agree with
the scalar path on the same inputs.
Configuration and dispatch
powl_policy.hpp decide which
long doublepath is taken. A wrong detection sends every
powlcall down the wrong kernel, so theformat-detection logic is worth careful eyes.
CCMATH_ENABLE_DETERMINISTIC): the claim is bit-identicalfloatanddoubleresults across supported hardware. Worth confirming it forces the generic kernels, disablesruntime SIMD, does not leak into a libm or compiler builtin on any path, and that constexpr
and runtime agree.
long doubleis not part of the deterministic claim.New public API
ccm::exthelpers under ext/ and the standardnextupand
nextdownunder fmanip/. Check signatures, NaN andinfinity behavior, constexpr-ness, and that they build clean under
-Wconversion. Theexthelpers are opt-in behind
CCMATH_ENABLE_EXTENSIONS, so also check the namespace placementand that they stay out of a default build.
Low-level support
cast.hpp,except_value_utils.hpp,fma.hpp. The changes here are small, but used very widely so mistake can easily propagate.Lower-risk areas (skim)
ccm::ppSIMD layer under runtime/pp/: astandalone C++17 port of the C++26
std::simdinterface. Not yet wired into dispatch, so itcannot change results this release. Review it on its own merits if you have the appetite,
otherwise it can wait.
question here is coverage gaps against the kernels above.
benchmarks. Build and dev infrastructure, not part of library runtime behavior.
meson.build,premake5.lua, the CMake options, and the vendored-consumerintegration test.
per-PR clang-format gate.
POW_PROOF.tex(the full binary64 proof is still open) and theSollya and Gappa scripts under
docs/approximating_functions.Validation status
What has been validated, and how. This lowers the uncertainty on these areas, it does not put
them out of bounds for review:
pow: CORE-MATH all-mode validation campaigns over the covered inputs, all four roundingmodes. The campaign records live under
tests/rigorous/(seeoracle_logs/for theper-campaign summaries).
sin,cos,tan: full-range validation campaigns, large arguments included. This isrepresentative coverage across the range, not an exhaustive sweep of every binary64 value.
fma: all-mode exact software fallback, with native runtime coverage validated on AArch64.nearest, power, and trig families.
-Werrorwith aggressive warnings (-Wconversionand friends) across the CImatrix on Linux, macOS, and Windows.
Open validation work (not expected to be finished in this RC):
powproof. The writeup in the tree is partial.powfreduced-domain validation.powlkernel.fmavalidation.Known limitations and intended scope
These are disclosed in the release notes and are intentionally out of scope for v0.3.0. Call
one out if you think it should block the release, otherwise treat it as known debt:
powlkernel is preview quality. Its generic fuzz lane is intentionally guardedoff in power_fuzz.cpp until the kernel is finalized.
Building and validating locally
It is a header-only library, so consuming it is just an include. To exercise the validation
that matters for review:
cmake -S . -B build -DCCMATH_BUILD_RIGOROUS_TESTS=ON \ -DCCMATH_ENABLE_WARNINGS_AS_ERRORS=ON -DCCMATH_ENABLE_AGGRESSIVE_WARNINGS=ON cmake --build build --config Release ctest --test-dir build --output-on-failure -C ReleaseOn multi-config generators like MSVC the
--configand-C Releaseflags matter, and theyare harmless elsewhere. Add
-DCCMATH_ENABLE_MPFR_TESTS=ONand-DCCMATH_ENABLE_COREMATH_TESTS=ONfor the oracle-backed campaigns (these need MPFR andCORE-MATH available, and the rigorous runs can take a while). Fuzzers build under Clang with
-DCCMATH_BUILD_FUZZING=ON. Runtools/ensure_format.shfor the format gate. The READMEvalidation section and
tools/asmlab/README.mdcover the deeper tooling.Where review is most valuable
Highest value first: numeric correctness and edge cases in the kernels above, then the public
API shape of the new
extandfmanipfunctions, then test coverage gaps against thosekernels. Style and tooling nits are welcome but lowest priority for an RC this size.