[NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

[Vitaly Davidovich]

On Thu, Mar 2, 2017 at 8:32 AM Ulf Zibis <Ulf.Zibis at cosoco.de> wrote:

> HI,
>
> first sorry for missing the class wide definition.
>
> Am 02.03.2017 um 12:45 schrieb Vitaly Davidovich:
> >
> > On Thu, Mar 2, 2017 at 6:38 AM Ulf Zibis <Ulf.Zibis at cosoco.de <mailto:
> Ulf.Zibis at cosoco.de>> wrote:
> >
> >     In any case, what's the reasonable of checking an argument, which is
> not used in that case?
> >
> > My understanding is contracts like the above (let's assume it still
> called out the NPE in 8) are
> > checked even if the argument isn't used in some cases.  The other case
> where I believe this is
> > done is when passing a Supplier to some method that uses it to obtain a
> default value - even if
> > it's not needed, it's checked for null because most (all?) such methods
> stipulate that it cannot
> > be null.
> On the other hand, the null check is a waste of performance and footprint
> (implicits performance
> cost too).

Implicit null checks aren't always eliminated (e.g. calling an empty
devirtualized method will still test the receiver for null) but generally
don't cost anything.  Explicit null checks can sometimes be folded into the
implicit one by the optimizer, but not always.

>
> I can't imagine, if any programmer would rely on such contract, instead of
> doing the check
> explicitly if of significance.

I think it's to have consistent behavior, irrespective of control flow.  It
can detect bugs early on.  I agree callers can do their own checks, but
this consistency is what I've noticed JDK to prefer.

>
>
> I think, the original reasonable of the contract is to rule out any other
> exception type or
> undefined behaviour in case of relevance of the argument. So I would vote
> for stating the contract
> more precisely in that way.
>
> Also, if I remember correct, if a variable, here replStr, is only
> referenced later, the compiler can
> move the execution of here replacement.toString() to a later position, so
> it is not clear to me, if
> the original implementation ever fulfilled the contract.

Compiler can only do that if it proves a bunch of things, such as no
observable side effects as a result of sinking it down.  If
CharSequence.toString isn't devirtualized there, eg, it definitely cannot
do it.  I wouldn't bet on it.

>
>
> -Ulf
>
> --
Sent from my phone