Re: [boost] [review][fixed_string] Andrzej's feedback

On 2019-12-05 12:05, Andrzej Krzemienski wrote:

czw., 5 gru 2019 o 09:59 Andrey Semashev via Boost mailto:boost@lists.boost.org> napisał(a):

On 2019-12-05 11:15, Alexander Grund via Boost wrote: > >> ``` >> void fixed_string<N>::resize(size_type s) >> { >>    BOOST_FIXED_STRING_PRECONDITION(s <= this->capacity()); >>    // then do the job >> } >> ``` > > +1 on that. I'm always advocating for safe-by-default and found it a > huge mistake to make operator[] the unchecked one instead of at() > > So using BOOST_FIXED_STRING_PRECONDITION which throws by default is the > right choice IMO.

I'm strongly opposed. Make it a BOOST_ASSERT if you like but no checks in release mode, please.

What's the point of this check when your index is guaranteed to not exceed size()-1?

In my whole programming practice, not once did I need at(). Not only because I didn't need the check at this point, but also because even if I did need a check at some point before operator[] call, I also was not satisfied with the exception at() would throw.

Are you opposing against the idea of user-controlled BOOST_FIXED_STRING_PRECONDITION() in general, or to throwing by default of to performing runtime-checks in release builds regardless of what action is taken later?

I'm strongly opposed to runtime checks regardless of the error outcome in release mode (ok, when NDEBUG is defined) because most of the time you already know the index is within bounds. When you don't know, you most likely should have check that earlier (e.g. before the parsing loop or before your higher level operation starts). I'm opposed to throwing an exception in case of failure because the exception and its error description is likely meaningless to high level code and nearly useless when debugging. The higher level code may not want an exception to begin with, in which case it would perform the check itself, and the check in operator[] becomes useless. I'm mildly opposed to using custom macros (other than NDEBUG) to modify behavior at compile time, because it opens up the possibility of configuration inconsistencies and ODR issues. I'm fine with it as long as by default (when nothing custom is defined) the behavior is equivalent to BOOST_ASSERT.

BOOST_ASSERT() does perform checks in release builds unless you go and define NDEBUG, which does not correspond 1-to-1 to release builds.

NDEBUG gets defined by default in release mode by many IDEs and build systems, it is the standard macro to disable asserts, including the standard assert(). In practice, NDEBUG and release mode nearly always go hand in hand and NDEBUG is perceived as an indication of a release build.