On Wed, 13 May 2026 at 19:02, Dragon Archer <[email protected]> wrote: > > 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.
Yes, I know this patch doesn't change the ryu code. But to add uses of __builtin_assume into the ryu code would require changing the ryu code. Just replacing all uses of assert with __builtin_assert is fine, but replacing them with __builtin_assume would not be fine, and so more specific changes to the ryu code would be needed. And I'm saying I wouldn't want such changes. You suggested it as something to consider, I'm saying it's not worth considering. > > > > > > > 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. >
