Make std.array.insertInPlace @safe for some cases

[tom-tan]

This pull request makes insertInPlace safe for some cases like:

• array for int[]

I leave the case for strings because the unittest for it uses std.conv.to which is currently not safe for strings.

[ghost]

I'm getting a little weary of these safety tricks that Kenji introduced but are now becoming more prevalent. Are we going to keep spamming lambda functions everywhere just to simulate safety? It really feels like the wrong approach.

Using this lambda trick also introduced a regression, so it has some issues with scoping. (although maybe that's just a compiler bug).

@andralex: Any opinions about these lambdas recently? Or am I worrying too much?

[Infiltrator]

I agree. What's the point of even having @safe if you're going to work around it anyway? The nothrow pure @safe fixes have to start at the core and work their way up to phobos.

[monarchdodra]

I'm getting a little weary of these safety tricks that Kenji introduced but are now becoming more prevalent. Are we going to keep spamming lambda functions everywhere just to simulate safety? It really feels like the wrong approach.

Are you complaining about the fact that we have functions that are safe that weren't before? Or the "hackish" approach of doing it via lambdas ?

I suggested a little while ago, doing the same thing with a more idiomatic "blocks with attributes".
http://forum.dlang.org/thread/pnxzjenvyzynsagwklxr@forum.dlang.org

I agree. What's the point of even having @safe if you're going to work around it anyway?

Isn't that what @trusted is all about? That said, I think that any function marked trusted should be reviewed much more closely, or it does undermine the entire thing.

[tom-tan]

Currently there is a performance issue to use trusted lambda call and 9rnsr proposed a pull request to solve this issue (See Issue 2483 in dmd: dlang/dmd#2483).

Can I wait for issue 2483 merged or use nested functions instead of lambda call?

[monarchdodra]

I think the implementation is way too lacking to be able to promise safety. If one of the calls to emplace throws, then the passed in slice will be modified to contain elements that were duplicated, but without a call to postblit, which can later lead to very nasty some very memory corruption issues. You could fix this by making a temp slice, and only assign back to the original slice at the end of the operation. Such a fix would be necessary.

Also, as I mentioned in std.array.array, emplace, as is, simply can't be used in safe code. Lucky for us, the ".ptr" should prevent that.

I suggest you change the pointer iteration for simple index iteration. This means the only line of code that would be unsafe is emplace(&src[i], val). From there, you could use the same trick as in std.array.array, to bypass emplace entirely when possible, making (at least) insertInPlace safe for the more basic types.

[tom-tan]

Sorry for late reply.

You could fix this by making a temp slice, and only assign back to the original slice at the end of the operation.

I found several things for that.

• Simple assigning back does not work for the structs with an elaborate destructor. In fact, such change breaks one of the unittests because of this issue. (See the unittest in https://github.com/tom-tan/phobos/blob/c629e5579926484df1391bb7426556cc43fc7e0c/std/array.d#L1168)
• In the current implementation of insertInPlace, we use copyBackwards but what we want to do is moving elements rather than copying them.
• I think std.algorithm.move helps us to this purpose. Unfortunately, it cannot work as safe function (currently) and it is not CTFEable.
• (Minor issue) I think any destructors of the struct should work even for the default value (i.e. T.init) but struct Int (which is defined in the unittest) does not work for the default value. If we use move instead of copyBackwords, it will cause segfalt.

First, I will make std.algorithm.move safe and CTFEable.
After that, I will fix this issue.

[tom-tan]

I filed one bug in druntime (Bugzilla: http://d.puremagic.com/issues/show_bug.cgi?id=11041).
Non-CTFEable core.stdc.string.memcpy prevents std.algorithm.move from being CTFEable
and it prevents insertInPlace from reimplementing with move without breaking its API.

[monarchdodra]

You can usually "bypass" these kinds of problem with "if (__ctfe)", where you "run" simpler code in case of ctfe.

Further more, you should mostly consider simplicity over efficiency for ctfe (after all, the code is never "run"). Bypassing "move" entirelly in copy backwards should be fine.

[tom-tan]

Thank you for your advice.
I will bypass move in copyBackwards.

BTW, should insertInPlace work for the struct with @disable opAssign? Currently the following code doesn't work.

    struct T
    {
        int a;
        @disable typeof(this) opAssign(typeof(this));
    }
    T[] a = [ T(1), T(2), T(3), T(4) ];
    a.insertInPlace(2, [ T(1), T(2) ]);
    assert(a == [ T(1), T(2), T(1), T(2), T(3), T(4) ]);

[dnadlinger]

@AndrejMitrovic: Regarding @trusted for parts of a function, that's not exactly a new discussion: http://forum.dlang.org/thread/blrglebkzhrilxkbprgh@forum.dlang.org. Maybe the time has come to finally do something about this?

[tom-tan]

Now insertInPlace bypass the emplace for most cases.

Some notes:

• I use a named trusted function instead of trusted lambda call in copyBackwards because currently it is not inlined.
• I define a nested function in insertInPlace to encapsulate the assignment. It prevents a duplication of the bypass for emplace.
• I leave the case for the struct with @disable opAssign (Currently it doesn't work with insertInPlace. It doesn't break the compatibility).
  If it should be work with insertInPlace, I will fix it.
• I remove the workaround for bug 8740 in a unittest. It is already fixed.

[dnadlinger]

Wait, why the extra GC allocation? Or am I missing something (just looking at the diff right now)?

[monarchdodra]

I think it's a "quick and dirty" way to achieve strong exception safety: If one of the insertions go wrong, then the original range is never modified.

[monarchdodra]

In which case, that array.length should but moved down...

[monarchdodra]

BTW, when allocating a temp array/buffer like this, you are usually better of using uninitializedArray (or minimallyInitilaizedArray).

[tom-tan]

Thank you for your comments and advice.
I moved down array.length and now insertInPlace uses uninitializedArray to allocate buffer.

[monarchdodra]

Looking better :)

[tom-tan]

If there are no problem, I will squash the commits.

[monarchdodra]

Looks good. Indeed, please squash.

[tom-tan]

I squashed the commits.

[tom-tan]

Oh...
Sorry for bad rebase.
I will fix it soon.

[tom-tan]

Done.
I rebased it.

[monarchdodra]

Merged. TYVM.