that duplicates a lot of code, violating DRY principal.

this would be much simpler:

replace { arg find, replace;
    ^super.replace(if(find.isKindOf(Char), find.asString, find), replace).join
}

just convert them in sclang.

Would be slower. Doesn't fix the issue with find. If you're concerned about some duplication you could probably refactor the primitives to avoid that.

We agreed when this was previously discussed that the primitives were the place to do this stuff.

yes, but it worked before. so why would it need so much extra code and an entirely different branch of an if statement to make it work again ?

the only issue is that it checks type and throws and error.

 if (!isKindOfSlot(b, class_string)) return errWrongType;

isn't that all that you added before ? chars are just strings with length 1

"chars are just strings with length 1" - Chris chars are not strings with length 1. they simply aren't. what makes you say that?

The thing is in SC chars are stored directly in u, whereas strings require a pointer. This means the search has to be handled slightly differently. If you look, there's not as much duplication as you might think. I actually started with a version trying to minimise duplication, but because of this difference it gets slightly tricky and harder to read. So I changed to the version in this PR, which I think is readable/maintainable, and not that long IAC.

@danstowell missing question mark at the end. it was a question.

I'm just looking through the commit and inquiring why there seems to be so much changed when it is supposed to just restore the previous functionality. Is that so bad that I'm asking ? More code introduces more areas for bugs. Its an entirely reasonable thing for me to inquire about. That's what code review is for.

@crucialfelix code review is good. There's nothing in my comment that implies otherwise. But when your question mark is missing, all that an outside observer can see is a misleading statement!

My understanding is that this PR is not supposed to restore previous functionality: correct me if I'm wrong but previous functionality was judged to be overly permissive about types (I think it was you that mentioned the nil example), and now @muellmusik is implementing what seems to be a consensus position, which unlike the previous code is not "anything goes".

@muellmusik thanks for the explanation

@muellmusik thanks for the explanation

No worries! If we could tighten the primitives up, I'd be happy to do that. I can take another look.

Okay, I haven't tested this, but I think something like this might work:

int prString_Find(struct VMGlobals *g, int numArgsPushed)
{
    PyrSlot *a = g->sp - 3; // source string
    PyrSlot *b = g->sp - 2; // search string
    PyrSlot *c = g->sp - 1; // ignoreCase
    PyrSlot *d = g->sp;     // offset

    int offset, alength, blength;
    int err = slotIntVal(d, &offset);
    bool bIsString = false;
    if (err) return err;

    alength = slotRawObject(a)->size - offset;

    if (isKindOfSlot(b, class_string)) {
        blength = slotRawObject(b)->size;
        bIsString = true;
    } else if (IsChar(b)) {
            blength = 1;
    } else return errWrongType;

    if ((alength <= 0) || (blength == 0)
                // should also return nil if search string is longer than source
            || (blength > alength))
        {
            SetNil(a);
            return errNone;
        }

    int cmp = 1;    // assume contains will be false
    char *achar, *bchar;
    char bchar0;
    achar = slotRawString(a)->s + offset;
    if(bIsString) {
        bchar = slotRawString(b)->s;
        bchar0 = bchar[0];
    } else { bchar0 = slotRawChar(b) }
        int scanlength = alength - blength;
        if (IsTrue(c)) {
            bchar0 = toupper(bchar0);
            for (int i=0; i <= scanlength; ++i, ++achar) {
                if (toupper(*achar) == bchar0) {
                    if(bIsString) {
                        cmp = memcmpi(achar+1, bchar+1, blength-1);
                    } else cmp = 0;
                        if (cmp == 0) break;
                }
        } else {
            for (int i=0; i <= scanlength; ++i, ++achar) {
                if (*achar == bchar0) {
                        if(bIsString) {
                         cmp = memcmp(achar+1, bchar+1, blength-1);
                        } else cmp = 0;
                    if (cmp == 0) break;
                }
            }
        }
        if (cmp == 0) {
            SetInt(a, achar - slotRawString(a)->s);
        } else {
            SetNil(a);
        }
        return errNone;
}

This removes the separate branch for char so is shorter, but will be slower with all the extra conditionals, and has some unused vars in the case of chars.

looks good

If it looks good, let's merge it so we don't have any more waste-of-time "quarks are broken" threads on the mailing list, reporting the same error that this fixes...?