> > I have a weird bug where concatenation is sometimes failing, and I
> > have no idea why. See the attached pasm. I fully expect both works and
> > weird to output "foo", "bar", "quux" with various levels of spacing,
> > but weird doesn't output quux.
>
> Patch below should fix the problem. This is not an optimal solution,
> as the unmake_COW is probably not required if the string_grow is
> going to happen anyway, but it seems to follow the general spirit of
> the current code.
Ah, this would be my bug. Thanks for finding it Peter. Unfortunately, I
fail to see why this actually fixes any bug. string_grow should unmake_COW
itself. So the old code essentially looked like this:
/* make sure A's big enough for both */
if (a->buflen < a->bufused + b->bufused) {
unmake_COW(interpreter,a);
/* Don't check buflen, if we are here, we already checked. */
Parrot_reallocate_string(interpreter, a, a->bufused + b->bufused + EXTRA_SIZE);
}
unmake_COW(interpreter, a);
While the new code with your patch, should look like:
unmake_COW(interpreter, a);
/* make sure A's big enough for both */
if (a->buflen < a->bufused + b->bufused) {
unmake_COW(interpreter,a);
/* Don't check buflen, if we are here, we already checked. */
Parrot_reallocate_string(interpreter, a, a->bufused + b->bufused + EXTRA_SIZE);
}
Since unmake_COW is a no-op if the string is not COW, I fail to see what
the functional difference is between these two snippets of code, although
I don't doubt that there is one if your fix solves Leon's code.
Also, I agree that unmake_COW is not an optimal function call if you're
going to grow the string afterwards. I wanted to get a simple interface
implemented for using COW, such that it would be easy to understand, and
then later optimize it for actual usage scenarios. I imagine that
unmake_COW could be extended to take 'pre' and 'post' byte arguments, and
would pad the resulting string by that much on either side when
uncowifying it. This would help string_grow optimally uncowify things. I
just haven't gotten around to that yet.
Thanks,
Mike Lambert
