ned14 / outcome

Provides very lightweight outcome<T> and result<T> (non-Boost edition)
https://ned14.github.io/outcome
Other
675 stars 62 forks source link

Use of `assert(false)` in `outcome::detail::make_ub` may cause significant code-bloat #294

Closed burnpanck closed 6 months ago

burnpanck commented 6 months ago

It appears that the assert I got shipped with my gcc-arm-none-eabi (embedded 32bit Arm) tries to be helpful, and generates a call like __assert_func(__FILE__, __LINE__, __PRETTY_FUNCTION__). Within template <class T> void make_ub(T &), __PRETTY_FUNCTION__ expands to a string that contains the fully demangled name of T, which may potentially be very long. In my specific case, that amounted to 150 kB of strings for a firmware that was otherwise ~100 kB big. Trying to fit a flash size of 256 kB, that is a challenge. While these strings obviously go away by defining NDEBUG, for a vocabulary type like outcome, it may be advisable to offer the user a method to avoid that bloat in builds without NDEBUG.

Maybe there could be a way to configure that specific use of assert, either to a custom implementation (without __PRETTY_FUNCTION__) or potentially disable it completely?

ned14 commented 6 months ago

How would opting into getting it C mangling to eliminate the bloat?

burnpanck commented 6 months ago

Not sure that I fully understand. Are you suggesting to declare make_ub as extern "C"? I'm not sure that this is going to help (nor am I sure that it would work at all). The source of bloat are the actual strings which are generated by __PRETTY_FUNCTION__, which get linked into the binary. __PRETTY_FUNCTON__ probably remains mostly the same independent of the linkage of the function.

My suggestion (actually, Peter Dimov's on Slack cpplang) would be to simply use a OUTCOME_ASSERT macro instead of the plain assert, and let users override that one.

ned14 commented 6 months ago

__PRETTY_FUNCTION__ would become a lot shorter if invoked from with a C function.

The OUTCOME_ASSERT is a good idea, so I'm minded to do both. Will need to wait until after the current Boost release however.

ned14 commented 6 months ago

That's fixed. Thanks for the BR!