Re: make OUString::copy() clip

On 08/02/2012 02:16 PM, Eike Rathke wrote:
> On Wednesday, 2012-08-01 22:02:50 +0200, Michael Stahl wrote:
> > On 01/08/12 19:05, Eike Rathke wrote:
> > > On Wednesday, 2012-08-01 18:17:12 +0200, Stephan Bergmann wrote:
> > > > make rtl::OUString::copy(beginIndex, count) clip to [0..length)?
> > > > yeah, why not  (then again, -1's sentinel nature, cf. indexOf, might
> > > > mean that silent clipping of beginIndex=-1 to beginIndex=0 is
> > > > unfortunate)
> > >
> > > We may step into more of these traps in transitions from String to
> > > OUString, so clipping IMHO is good.
> > >
> > > I think beginIndex<0 or count<=0 should always return an empty string
> > > and output a SAL_WARN, maybe also SAL_WARN if clipping occurred as that
> > > may indicate a logic error.
> >
> > no, it should assert().  passing in invalid indexes is clearly a bug
> > that must be fixed.
>
> Well, yes, but the assert() hits only with debug, in non-debug build the
> copy() happily (probably depending on memory layout) may copy excess
> characters on Linux while it may crash on Windows or Mac. So for many
> developers it may still go unnoticed.

...which puts the blame on those developers...

> My suggestion then: keep the assert() for debug/dbgutil heroes but clip
> thereafter for when the assert() is not active.

No "defensive programming," please.  If we consider a given (beginIndex, 
count) arguments a violation of copy's precondition, then fail on it, as 
fast as possible (via assert -> abort in a non-NDEBUG build, via 
undefined behavior (-> hopefully crash soon) otherwise).

Which leaves the question of how to specify copy's precondition.  Today, 
"[it] is an error for either beginIndex or count to be negative, or for 
beginIndex + count to be greater than the length of this string."

It has rightfully been observed that it might be helpful for client code 
if the case of beginIndex + count > length were accepted by copy, and 
handled by clipping the arguments to (beginIndex', count'), where 
beginIndex' = min {beginIndex, length} and count' = min {count, length - 
beginIndex'}.

Whether it would also be helpful to accept other currently invalid 
(beginIndex, count) arguments (that would involve beginIndex and/or 
count being negative) is open to debate.

Stephan