Fix issue 14925 -- narrow strings should not match the first overload of replaceInPlace.

[schveiguy]

Also, fixed issues with non-compatible arrays matching the first overload.

Note the rewritten constraint of the second version. It was getting very hairy, so I just decided on a strategy of "it doesn't match the first, and I can call replace, so do it"

[9rnsr]

I think we don't have to keep two overloaded functions for replaceInPlace.

The purpose of the first overload of replaceInPlace is for the optimization.
And, replaceInPlace itself would take equivalent template arguments that the replace function takes.
So I think following code style would be simpler:

void replaceInPlace(T, Range)(ref T[] array, size_t from, size_t to, Range stuff)
if (is(typeof(replace(array, from, to, stuff))))
// <-- `replaceInPlace` has equivalent constraint with the one that `replace` has
{
    static if ( isMutable!T &&
               !isNarrowString!(T[]) &&
                isDynamicArray!Range &&
                is(Unqual!(ElementEncodingType!Range) == T))
    // <-- elabolate internal constraint for the optimized code
    {
        // [snip]
        // <-- the body of first overload
    }
    else
    {
        array = replace(array, from, to, stuff);
        // <-- the body of second overload
    }
}

/// Ditto   <-- can be removed
//void replaceInPlace(T, Range)(ref T[] array, size_t from, size_t to, Range stuff)
//{
//}

[schveiguy]

Yes, I think this is an appropriate way to refactor, will do.

[schveiguy]

Done

[schveiguy]

Some whitespace changes, use this link to view without the whitespace changes: #3561

[9rnsr]

I think an unlisted bug exists here. If T is inout type, it would accidentally go into this optimized pass.
I'd suggest to use isMutable!T.

[9rnsr]

But, it can be separated issue. I'd go forward this improvement PR.

[schveiguy]

I wrote this in unit test, and it didn't fail:

    void foo(inout(int)[] x)
    {
        x.replaceInPlace(1,2, [1,2,3]);
    }

Not sure if I did that right, I agree it probably should match the static if.

[schveiguy]

Hah, I know why it works :) In fact, I think possibly we don't even need the isMutable: the Unqual call will make sure I am testing the mutable version of the element type against T, and this will only succeed if T is mutable. A happy accident!

[9rnsr]

Yes, I also confirmed that. So we can also remove the check !is(T == const T) && !is(T == immutable T). Hooray!

[9rnsr]

I opened #3565 for the supplemental fix.

[9rnsr]

Auto-merge toggled on