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

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2019-12-05 09:30:43

On 2019-12-05 12:05, Andrzej Krzemienski wrote:
> 
> 
> czw., 5 gru 2019 o 09:59Â Andrey Semashev via Boost 
> <boost_at_[hidden] <mailto:boost_at_[hidden]>> 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.