implement dup and idup library functions #760

MartinNowak · 2014-04-05T10:32:30Z

uses inferrence for pure and nothrow
allocates uninitialized memory using _d_newarrayU

monarchdodra · 2014-04-05T16:41:17Z

src/object_.d

+        return 1;
+    }
+
+    int dg2() pure nothrow @safe //Fallback to typeid


I don't think this "Fallback to typeid" comment is still relevant?

monarchdodra · 2014-04-05T16:55:20Z

Awesome pull. I like it a lot.

The only thing I regret (if I understood correctly), is that the inference happens thanks to CTFE. However, the actual implementation uses tid's run-time postblit, which is "pre-preemptively" marked as safe, pure nothrow. What worries me is that if the postblit does throw, what will happen, given the exception comes from a the non-throwing _dup functions ...

But this is a generic problem of typeinfo, not this specific implementation.

MartinNowak · 2014-04-05T16:58:14Z

I think it needs a little more tweaking, the inferrence should come from casting the postblit delegate in _getPostblit, but using @trusted breaks it.

MartinNowak · 2014-04-05T16:59:31Z

src/object.di

+{
+    // @@BUG 12523@@ wrong foreach argument type with ref and inout
+    version (none) if (__ctfe)
+    {


In the meantime I've submitted a pull to fix this issue dlang/dmd#3424.

MartinNowak · 2014-04-05T20:38:22Z

Lets fix dlang/dmd#3424 first, then we can probably achieve strong purity using inout.
Also using overloads might be a better approach than nested @trusted functions.

_Unconst!T[] dup(T)(T[] a) @trusted
    if (is(typeof(() @safe { T a = T.init, b = a; })))
{}

_Unconst!T[] dup(T)(T[] a) @system
    if (!is(typeof(() @safe { T a = T.init, b = a; })) &&
        is(typeof(() @system { T a = T.init, b = a; })))
{}

monarchdodra · 2014-04-05T20:55:18Z

Also using overloads might be a better approach than nested @trusted functions.

The overload idea seems like nice idea. I like it. But I'd really hate exposing a user to such an overload. It should be implementation detail, rather than public interface. How about:

_Unconst!T[] dup(T)(T[] a)
{
    static if (is(typeof(() @safe { T a = T.init, b = a; })))
        return _dupTrusted(a);
    else
        return _dup(a);
}

_Unconst!T[] _dupTrusted(T)(T[] a) @trusted
{
    return _dup(a);
}

Well, it's still basically just a "nested trusted" that was un-nested, but there's only one now. _dup is then free to be as system as it wishes.

Also, I think the code can be made shorter with (T b) @safe { T a = b; }

But whatever gets the job done is fine by me.

MartinNowak · 2014-04-05T21:14:37Z

Good ideas, will try them tomorrow.

MartinNowak · 2014-04-06T17:06:57Z

Just hit another compiler bug Issue 12528. Also it's currently not possible to cast away const/immutable/inout in CTFE.

monarchdodra · 2014-04-08T18:20:55Z

I haven't reviewed the code in lifetime (not my domain), but what I saw in object.d looks good. In any case, it's consistent with what we'd have used in Phobos (bar some extra traits, static if (hasElaborateAssign)).

I am particularly thrilled see that the code was consolidated into an internal.traits module.

Let's have someone else review this too, and get it shipped!

MartinNowak · 2014-04-08T19:30:18Z

I am particularly thrilled see that the code was consolidated into an internal.traits module.

It's a necessity more than a feature, I'd still want to keep the amount of compile time reflection on druntime low because it might easily slow down compilation or increase binary sizes.

WalterBright · 2014-04-10T18:30:26Z

src/rt/lifetime.d

                {
                    if(*length == cast(ubyte)oldlength)
                        *length = cast(ubyte)newlength;
                    else
                        return false;
                }
+                catch (Throwable t)


Huh? Why would synchronized ever throw?

Because the lock/unlock functions of the Monitor interface aren't nothrow. IMO they should be, but it could break code. Maybe we can find a way to deprecate the missing nothrow.

WalterBright · 2014-04-10T19:16:57Z

Note that the following code should be successful with this PR:

int[] test6(int[] a) pure @safe nothrow
{
    return a.dup;
}

/***********************************/

int*[] pureFoo() pure { return null; }


void testDIP29_5() pure
{
    { char[] s; immutable x = s.dup; }
    { immutable x = (cast(int*[])null).dup; }
    { immutable x = pureFoo(); }
    { immutable x = pureFoo().dup; }
}

I understand that the compiler changes will still be needed to call the library function rather than adDup(), but this can be tested by temporarily naming things as Dup rather than dup.

MartinNowak · 2014-04-10T21:57:03Z

but this can be tested by temporarily naming things as Dup rather than dup

Thanks for the additional test cases, using a.dup() will work as well.

WalterBright · 2014-04-11T03:24:37Z

@MartinNowak I'd like to get this one merged ASAP, so I can excise the corresponding ugly code out of the compiler.

I also think a PR to change std.traits to use core.internal.traits would be great, to avoid code duplication.

monarchdodra · 2014-04-11T06:20:36Z

I also think a PR to change std.traits to use core.internal.traits would be great, to avoid code duplication.

I'm fine with that. We can do it as soon as this is merged.

monarchdodra · 2014-04-11T06:34:50Z

src/rt/lifetime.d

+    debug(PRINTF) printf(" p = %p\n", info.base);
+    // update the length of the array
+    auto arrstart = __arrayStart(info);
+    memset(arrstart, 0, size);


Why are you doing a memset in an uninitialized allocation? It's duplicate work from what goes on in _d_newarrayiT. Did you just forget to remove it?

If anything, if BlkAttr.NO_SCAN is not set, then I'd expect a memset of memset(arrstart+size+pad, 0, info. size-size-pad).

Reading though rt.lifetime, this seems like something we never used to do anyways. Does the GC "know" it doesn't have to scan that data?

Oops, that's a copy paste mistake.

monarchdodra · 2014-04-11T06:38:32Z

Could you also add some unittest, that the result of a dup has capacity? EG:

auto a = [1, 2, 3];
auto b = a.dup();
assert(b.capacity >= 3)

Speaking of which, Phobos would love having access to an un-initialized allocation function, that has appendable info: https://issues.dlang.org/show_bug.cgi?id=12444 . But that's beyond the scope of this pull.

- rewrite _d_newarrayT and _d_newarrayiT in terms of _d_newarrayU - mark all _d_newarray functions as pure nothrow - optimize initialization in _d_newarrayiT

@safe

- can infer @safe, pure and nothrow - strongly pure dup if element type implicitly convertible to mutable - allocates uninitialized memory using _d_newarrayU

MartinNowak · 2014-04-11T20:35:15Z

Updated to hopefully address all the remaining issues.

MartinNowak · 2014-04-11T20:42:41Z

I'm a bit disappointed that the initial idea turned into 90 lines of convoluted template code.
Almost feels like writing C++ code for boost.

WalterBright · 2014-04-11T22:48:35Z

Thank you. Well done!

implement dup and idup library functions

monarchdodra · 2014-04-13T11:26:13Z

Thank you. Well done!

Indeed. The final result is somewhat "convoluted", but it works, and there's no duplicate code.

May I bring to attention this issue: https://issues.dlang.org/show_bug.cgi?id=12512
It is (I think) quite related (in terms of implementation). In any case, it's dup related, and blocker for me for use in generic code.

WalterBright · 2014-04-15T19:10:24Z

This introduced regression https://issues.dlang.org/show_bug.cgi?id=12580

monarchdodra · 2014-05-04T21:15:28Z

Hey... I was trying to duplicate the "static postblit cast" thingy in a phobos, and I don't think it's working at all. I think the only reason this is working is because of the ctfe appends. If I remove it, then the attribute tests start failing.

I don't think the whole alias PostBlitT = ... thing is working. Look at this:

struct T
{
    this(this) @system
    {}
}

void main()
{
    alias PostBlitT = typeof(function(void*){T a = T.init, b = a;});
    pragma(msg, PostBlitT.stringof);
}

gives me:

void function(void* _param_0) pure @nogc @safe

But this(this) is not any of those attributes. This is a compiler bug... right? The function type is not correctly inferred here? Or is there some other rule i regards to functions/delegates?

ghost · 2014-05-04T21:36:23Z

How does that even compile without b being defined? There may be multiple bugs here.

ghost · 2014-05-04T21:41:47Z

Note that there seems to be a difference when using typeof() or not. For example, after applying my fixup pull the behavior is:

struct T
{
    this(this) @system
    {}
}

void main()
{
    alias PostBlitTypeof = typeof(function(void*){T a = T.init, b = a;});
    auto PostBlit = function(void*){T a = T.init, b = a;};

    pragma(msg, __traits(getFunctionAttributes, PostBlitTypeof));
    pragma(msg, __traits(getFunctionAttributes, typeof(PostBlit)));
    pragma(msg, __traits(getFunctionAttributes, PostBlit));
}

tuple("pure", "@nogc", "@safe")
tuple()
tuple("@system")

It's quite strange. But essentially, typeof seems to create problems.

monarchdodra · 2014-05-05T06:25:25Z

How does that even compile without b being defined? There may be multiple bugs here.

Note the comma, basically, we are defining b. EG:

int a = 5, b = a;

ghost · 2014-05-05T08:10:33Z

For a second there I thought it was another usage of the comma operator (well it is, but not the other evil comma operator :P).

MartinNowak · 2014-05-05T19:54:12Z

So you're saying we need to use this workaround.

    enum PostBlit = function(void*){T a = T.init, b = a;};
    alias PostBlitT = typeof(PostBlit);

Issue 12704 – typeof function literal incorrectly infers attributes
Issue 12561 – typeof function literal doesn't check @safe

monarchdodra · 2014-05-05T20:57:40Z

Question: Can phobos access package functions in core? I need this for phobos, so I want to know the "cleanest" way?

It would only be "temporary", since the idea would be for phobos to one day have a function that statically postblits without any run-time overhead, but still, I think there's enough duplication already.

monarchdodra · 2014-05-05T20:58:55Z

a function that statically postblits without any run-time overhead

By that I meant "a function that statically knows how to postblit, without runtime introspection".

ghost · 2014-05-05T22:28:04Z

Question: Can phobos access package functions in core?

Nope, package has its issues right now making it not too useful.

dnadlinger · 2014-05-06T08:57:57Z

Also, Phobos should be "just another library", and not get any special access to the druntime internals.

MartinNowak restored the dup branch April 5, 2014 10:32

MartinNowak mentioned this pull request Apr 5, 2014

step one of moving .dup to library #758

Closed

monarchdodra reviewed Apr 5, 2014
View reviewed changes

MartinNowak reviewed Apr 5, 2014
View reviewed changes

WalterBright reviewed Apr 10, 2014
View reviewed changes

monarchdodra reviewed Apr 11, 2014
View reviewed changes

MartinNowak added 4 commits April 11, 2014 22:32

add Unqual to core.internal.traits

ebee7fd

make __setArrayAllocLength pure nothrow

0b30e34

add _d_newarrayU to allocate an uninitialized array

c2bea90

- rewrite _d_newarrayT and _d_newarrayiT in terms of _d_newarrayU - mark all _d_newarray functions as pure nothrow - optimize initialization in _d_newarrayiT

implement dup and idup library functions

a403169

- can infer @safe, pure and nothrow - strongly pure dup if element type implicitly convertible to mutable - allocates uninitialized memory using _d_newarrayU

WalterBright added a commit that referenced this pull request Apr 11, 2014

Merge pull request #760 from MartinNowak/dup

a6cbeef

implement dup and idup library functions

WalterBright merged commit a6cbeef into dlang:master Apr 11, 2014

MartinNowak deleted the dup branch April 12, 2014 07:24

ibuclaw mentioned this pull request Sep 24, 2014

Fix extra case where goto skips initialization of variables #965

Merged

dkorpel mentioned this pull request May 17, 2021

Rewrite object.dup to work with scope and copy constructors #3474

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement dup and idup library functions #760

implement dup and idup library functions #760

MartinNowak commented Apr 5, 2014

monarchdodra Apr 5, 2014

monarchdodra commented Apr 5, 2014

MartinNowak commented Apr 5, 2014

MartinNowak Apr 5, 2014

MartinNowak commented Apr 5, 2014

monarchdodra commented Apr 5, 2014

MartinNowak commented Apr 5, 2014

MartinNowak commented Apr 6, 2014

monarchdodra commented Apr 8, 2014

MartinNowak commented Apr 8, 2014

WalterBright Apr 10, 2014

MartinNowak Apr 12, 2014

WalterBright commented Apr 10, 2014

MartinNowak commented Apr 10, 2014

WalterBright commented Apr 11, 2014

monarchdodra commented Apr 11, 2014

monarchdodra Apr 11, 2014

MartinNowak Apr 11, 2014

monarchdodra commented Apr 11, 2014

MartinNowak commented Apr 11, 2014

MartinNowak commented Apr 11, 2014

WalterBright commented Apr 11, 2014

monarchdodra commented Apr 13, 2014

WalterBright commented Apr 15, 2014

monarchdodra commented May 4, 2014

ghost commented May 4, 2014

ghost commented May 4, 2014

monarchdodra commented May 5, 2014

ghost commented May 5, 2014

MartinNowak commented May 5, 2014

monarchdodra commented May 5, 2014

monarchdodra commented May 5, 2014

ghost commented May 5, 2014

dnadlinger commented May 6, 2014

implement dup and idup library functions #760

implement dup and idup library functions #760

Conversation

MartinNowak commented Apr 5, 2014

monarchdodra Apr 5, 2014

Choose a reason for hiding this comment

monarchdodra commented Apr 5, 2014

MartinNowak commented Apr 5, 2014

MartinNowak Apr 5, 2014

Choose a reason for hiding this comment

MartinNowak commented Apr 5, 2014

monarchdodra commented Apr 5, 2014

MartinNowak commented Apr 5, 2014

MartinNowak commented Apr 6, 2014

monarchdodra commented Apr 8, 2014

MartinNowak commented Apr 8, 2014

WalterBright Apr 10, 2014

Choose a reason for hiding this comment

MartinNowak Apr 12, 2014

Choose a reason for hiding this comment

WalterBright commented Apr 10, 2014

MartinNowak commented Apr 10, 2014

WalterBright commented Apr 11, 2014

monarchdodra commented Apr 11, 2014

monarchdodra Apr 11, 2014

Choose a reason for hiding this comment

MartinNowak Apr 11, 2014

Choose a reason for hiding this comment

monarchdodra commented Apr 11, 2014

MartinNowak commented Apr 11, 2014

MartinNowak commented Apr 11, 2014

WalterBright commented Apr 11, 2014

monarchdodra commented Apr 13, 2014

WalterBright commented Apr 15, 2014

monarchdodra commented May 4, 2014

ghost commented May 4, 2014

ghost commented May 4, 2014

monarchdodra commented May 5, 2014

ghost commented May 5, 2014

MartinNowak commented May 5, 2014

monarchdodra commented May 5, 2014

monarchdodra commented May 5, 2014

ghost commented May 5, 2014

dnadlinger commented May 6, 2014