On Wed, 13 May 2026 at 16:46, Jonathan Wakely <[email protected]> wrote: > On Wed, 13 May 2026 at 16:14, Dragon Archer <[email protected]> wrote: > > > > Thank you for your review and thoughtful questions. > > > > > Seems reasonable to me (though pragma push/pop_macro don't seem strictly > > > necessary as Jonathan pointed out). > > > > > > Though in theory I guess there's a risk of a performance regression by > > > removing an assert in Ryu, if the compiler relies on an assert condition > > > to be true in order to safely apply an important optimization. > > > > I understand the concern. In my view: > > > > 1. Replacing an unconditional `assert` (which always evaluates the > > condition at runtime) with `__glibcxx_assert` (which is completely removed > > when `_GLIBCXX_ASSERTIONS` is not defined) actually **removes** runtime > > checks in non‑debug builds. This performance benefit likely outweighs any > > hypothetical optimization that the compiler might have derived from the > > assertion. > > > > 2. If we are worried about missing optimization hints, we could consider > > using `__builtin_assume` – that would be more semantically precise. > > However, that would require guarding it with `#ifdef _GLIBCXX_ASSERTIONS` > > and defining a fallback. I personally prefer keeping the current approach > > with `__glibcxx_assert` because it is already used throughout libstdc++ and > > matches the debugging philosophy. > > I don't want such changes in the Ryu code, it will just make it > difficult to rebase on upstream. However, we might be able to replace > it with Teju Jagua anyway.
As discussed in PR/125228, this patch only changes libstdc++-v3/src/c++11/debug.cc and libstdc++-v3/src/c++17/floating_to_chars.cc, both are pure GCC files that are not shared with upstream. So there should be no rebase issues. > > > > 3. If the community later decides that `__glibcxx_assert` should also serve > > as an optimization hint in release mode, they could define it to > > `__builtin_assume` instead of empty when `_GLIBCXX_ASSERTIONS` is **not** > > defined. That would be a separate change, and will automatically be > > effective for this patch as well. > > That won't happen while I have any say in the matter. > I didn't want to propose that change either. > > > > So for this patch, I believe using `__glibcxx_assert` is the right and > > simple solution. > > > > > Just wondering, is there a legit problem/concern with __FILE__ appearing > > > in the final library or is it a matter of QoI (which I agree with)? > > > > It is primarily a QoI improvement. The presence of `__FILE__` strings > > increases binary size and may leak internal build paths (which is a minor > > privacy/security concern for some users). > > Users who care about that can easily compile in /tmp or another > non-secret path. You can also compile with --enable-cxx-flags=-DNDEBUG > to disable those assertions in your own builds. > Yes, that's true, but I think this patch is clean enough and can be made available in upstream without causing any issues. It is a small change that improves the quality of the library without affecting functionality (and may even improve performance in non‑debug builds). > > Eliminating them makes the library cleaner, but there is no functional > > correctness issue. BTW, I think the removal of runtime checks will improve > > perfomance slightly. > > > > Again, thank you for your time and feedback.
