Issue 7942 - Appending different string types corrupts memory

[yebblies]

This adds the runtime functions necessary to append any string type to any other string type.

The performance is probably not ideal but is also not a priority as the current situation causes memory corruption.

https://issues.dlang.org/show_bug.cgi?id=7942

[yebblies]

Must be merged before dmd pull: dlang/dmd#3831

[MartinNowak]

Why byte[]?

[yebblies]

Oops, copy-paste error from the existing code.

[yebblies]

Fixed.

[MartinNowak]

A unittest for the new functions in rt.lifetime would be nice :).

[yebblies]

I have unittests in the dmd pull. Are they really necessary here?

[MartinNowak]

I have unittests in the dmd pull. Are they really necessary here?

I'd really appreciate it. Keeping tests co-located to the source helps during development.
Something simple like this would suffice.

char[] chars = "chars";
wchar[] wchars = "wchars"w;
_d_arrayappendcwa(chars, wchars);
assert(chars == "charswchars");

[yebblies]

I really don't see the point, and I'd rather not. I always find unittests in druntime cumbersome to work with.

Updated to add redundant unittests.

[MartinNowak]

I really don't see the point, and I'd rather not.

It takes about 10 minutes to run dmd tests and they are part of a different repo.
This makes it unnecessarily harder to test changes in druntime.

I always find unittests in druntime cumbersome to work with.

Why? I'm not too happy with the Makefile tests, but I don't see what is wrong with D unittests.
The splitting is also pretty clear, druntime tests should check that functions implementations are correctly, dmd tests should check that the compiler<->rt interface works.

[MartinNowak]

Auto-merge toggled on

[yebblies]

Maybe it's just that I'm not used to working on druntime. These tests have to be in the dmd test suite, and dmd is the only caller of these functions. I'm much more comfortable running the dmd tests, and the way I have it set up it takes much less than 10 minutes.

And thanks.

[MartinNowak]

Thanks for fixing the bug :).

[yebblies]

Not fixed until the dmd pull is merged too!

[9rnsr]

Original issue is an accepts-invalid bug. Why specially allow transcoding on appending?

I'm afraid that implicit transcoding will be hidden cost addition to the gc operation and elements copying. If the appended string is very long, the cost will be bigger.

[yebblies]

I'm afraid that implicit transcoding will be hidden cost addition to the gc operation and elements copying. If the appended string is very long, the cost will be bigger.

The cost will still be O(N), as expected for array concatenation.

[yebblies]

Original issue is an accepts-invalid bug. Why specially allow transcoding on appending?

As I said in the bug report, it seems like a straightforward extension of the single char case. Isn't this a useful thing to do with unicode strings?

[MartinNowak]

As I said in the bug report, it seems like a straightforward extension of the single char case. Isn't this a useful thing to do with unicode strings?

I never needed or used it, that's why I think it won't introduce a performance issue.
Also note that it's more efficient to do concat a different string type than it is to first convert and then concat.
@yebblies will your code also allow to concat string literals of different type, because currently "foo"c ~ "bar"d is not allowed.

[yebblies]

@yebblies will your code also allow to concat string literals of different type, because currently "foo"c ~ "bar"d is not allowed.

No, but it's not as high priority because it isn't wrong-code.