[new AA] implement aaLiteral #933

IgorStepanov · 2014-08-22T19:40:59Z

This functions will be used to constructing AA values: literals and nulls. When this PR will be merged, I will create PR for dmd, which uses this functions, and when dmd PR will be merged, I'll create PR, which rewrite this functions to use new AA implementation.

yebblies · 2014-08-23T07:07:24Z

src/object.di

+            values ~= cast(Value)args[i+1];
+        }   
+
+        void[] key_slice = (cast(void*)keys.ptr)[0 .. keys.length];


Huh? What if Key.sizeof != (void*).sizeof ?

This is void slice, not void*. And yes, Key.sizeof can be not equals with void.sizeof. However _d_assocarrayliteralTX void[] takes slice with length == keys count. See rt/aaA.d:562:

Impl* _d_assocarrayliteralTX(const TypeInfo_AssociativeArray ti, void[] keys, void[] values) { ... const length = keys.length; ... assert(length == values.length); ... for (size_t j = 0; j < length; j++) { auto pkey = keys.ptr + j * keysize; auto pvalue = values.ptr + j * valuesize; ... } ... }

Oh yuck it's one of those APIs.

Yes :)
This is another one reason to replace old AA implementation.

I'd rather you did it via *(cast(void[]*)&keys) instead to make it clear it's a reinterpret cast.

IgorStepanov · 2014-08-26T19:39:56Z

Reworked. Now this PR introduce only aaLiteral, because my last AA implementation use zero AA init (similar old AA).

IgorStepanov · 2014-09-02T00:01:38Z

@MartinNowak ping

quickfur · 2014-09-16T14:17:56Z

Is the dmd pull ready yet, or were you planning to merge this first and submit the dmd changes later?

IgorStepanov · 2014-09-16T18:56:21Z

This PR should be merged first. After that, dmd pull should pass tests (they are passed on my machine). After dmd pull will be merged (of course, it should be reviewed first), I will create the next druntime PR. It will be simple and use AA implementation from #934 (which are also waitinig for review).
This work is done on my machine and passed tests.
And all stages of this work need LGTM from @MartinNowak and @yebblies, even if PR's will be reviewed by another one.

quickfur · 2014-09-16T19:02:22Z

OK, makes sense. I'll leave it to them to approve this PR then.

dnadlinger · 2014-09-19T09:42:01Z

src/object.di

+    void* _d_assocarrayliteralTX(const TypeInfo_AssociativeArray ti, void[] keys, void[] values) @trusted pure;
+}
+
+auto aaLiteral(Key, Value, T...)(T args) @trusted


Isn't @trusted a potential safety hole here if any of the argument types define a custom opCast?

Current AA implementation is one big hole:

void* p; struct MyKey { size_t a; size_t toHash() const { p = cast(void*)a; if (a == 5) throw new Exception("hello"); return a; } } void main() @safe pure nothrow { int[MyKey] aa; aa[MyKey(3)] = 3; aa[MyKey(5)] = 5; //BOOOM }

This code succesfully compiles and runs on dmd v2.066

This PR is a transitional stage between current implementation and new good, safe template-based implementation.
This PR doesn't brings something new. Where dmd inserts aaLiteral, it called _d_assocarrayliteralTX without any checks early.

Should this use auto ref T args to avoid temporary copies?

auto ref causes an unexpected troubles on some tests. May be it's a compiler bug, may be it is my bug in dmd code. If dlang/dmd#3904 will be reviewed by someone dmd guru like @9rnsr my possible bug can be found. But for review 3904 we need merge this code.
I think this is not big issue, because this code will be replaced with new AA fast enough (I hope until the next release).

Done. I've fixed bug in my dmd patch. Now we can use auto ref arguments in aaLiteral.

MartinNowak · 2014-09-20T20:14:01Z

src/object.di

+        Key[] keys;
+        Value[] values;
+        keys.reserve(T.length / 2);
+        values.reserve(T.length / 2);


auto keys = new Key[](T.length / 2); auto values = new Value[](T.length / 2);

No. If I'll construct a array compiler will initialize it with init data (is Value is a struct with @disable this() it causes a error).
After that I'll shoud to assign (call opAssign) instead of adding new values.

IgorStepanov · 2014-09-21T21:45:46Z

@MartinNowak I've applied changes that you asked, except setting array elements by index (I said why).
Also I've reworked and retested dlang/dmd#3904
I'll apply asked changes to #934 tomorrow, it need more thoughtful work.

IgorStepanov · 2014-09-22T00:38:11Z

@MartinNowak Please, rereview it.
BTW, what happens with FreeBSD server? This PR cant fails because it contains three templates, which never instatiated.

IgorStepanov · 2014-09-23T12:37:53Z

@MartinNowak ping

IgorStepanov · 2014-09-29T17:44:55Z

@MartinNowak may be this PR can be merged? It is so trivial and its merging allows to start review of dmd part. And when you will able to review #934 PR?

IgorStepanov · 2014-09-30T22:49:53Z

@MartinNowak ping

MartinNowak · 2014-10-01T08:51:21Z

src/object.di

+        foreach (i; staticIota!(0, args.length / 2))
+        {
+            keys ~= cast(Key)args[2*i];
+            values ~= cast(Value)args[2*i + 1];


Casting could be unsafe here, so remove the @trusted from above and let the compiler infer it.

Hmm. I've removed casts and tests have been passed. I can't remember, why I added them. =/

I can't remove @trusted, because void[] key_slice = *cast(void[]*)&keys; is unsafe. (It's part of old AA API)

MartinNowak · 2014-10-01T08:52:54Z

OK, let's get this going. I'm still not really happy with the template bloat caused the variadic template arguments, but we can optimize the details later.

IgorStepanov · 2014-10-01T21:56:20Z

@MartinNowak is it ok now?

IgorStepanov · 2014-10-03T20:02:29Z

@MartinNowak Please, if you see my changes but not ready to review this, tell me something like printf("I see this, but I haven't time/energy to review this now. Remind me in %s", time_until_remind)
I'm sorry that I bother all my PR's ping, but lack of response makes me think that my action banal not seen.

MartinNowak · 2014-10-04T07:11:38Z

I'm sorry that I bother all my PR's ping, but lack of response makes me think that my action banal not seen.

Well it isn't seen, because I only look at my github inbox when I have time.

MartinNowak · 2014-10-04T07:19:54Z

We still have to let the compiler infer safety because appending a struct to an array might call an unsafe postblit.
You can make the cast trusted by using a trusted function literal.

void[] key_slice, value_slice;
() @trusted {
    key_slice = *cast(void[]*)&keys;
    value_slice = *cast(void[]*)&values;
}();

MartinNowak · 2014-10-04T07:21:03Z

src/object.di

+    void* _d_assocarrayliteralTX(const TypeInfo_AssociativeArray ti, void[] keys, void[] values) @trusted pure;
+}
+
+auto aaLiteral(Key, Value, T...)(auto ref T args) @trusted


Let's add if (T.sizeof % 2 == 0) as template constraint.

IgorStepanov · 2014-10-04T10:26:02Z

You can make the cast trusted by using a trusted function literal.

Pretty syntax. Done.

IgorStepanov · 2014-10-04T11:26:25Z

@MartinNowak done. Also I've moved _d_assocarrayliteralTX call to @trusted delegate and removed @trusted from _d_assocarrayliteralTX signature.

MartinNowak · 2014-10-04T12:34:59Z

Pretty syntax. Done.

Yikes, I'm not even sure we can optimize away the function call.

MartinNowak · 2014-10-04T12:37:04Z

removed @trusted from _d_assocarrayliteralTX signature

Oh, hadn't seen that before. Please also add all declared attributes to the implementation, it probably cannot be pure.

IgorStepanov · 2014-10-04T12:48:47Z

Oh, hadn't seen that before. Please also add all declared attributes to the implementation, it probably cannot be pure.

It CAN be impure. And we can't check the purity.
This example works in current dmd version.

void* p;
struct MyKey
{
    size_t a;
    size_t toHash() const
    {
        p = cast(void*)a;
        if (a == 5)
            throw new Exception("hello");
        return a;
    }
}

void main() @safe pure nothrow
{
    int[MyKey] aa;
    aa[MyKey(3)] = 3;
    aa[MyKey(5)] = 5; //BOOOM
}

Compiler doesn't provide any check and inserts _d_assocarrayliteralTX into safe code without a doubt.
It's another one reason to migrate to new AA implementation as possible faster :)

IgorStepanov · 2014-10-04T13:25:46Z

In other words, we can't make _d_assocarrayliteralTX pure, because it can call impure hash function, and we can't make _d_assocarrayliteralTX impure, because aa listerals can be created in pure code. However I don't add any new holes in this PR. Current hole exists in old implementation, and we should move to new implementation to fix this hole.

MartinNowak · 2014-10-04T13:26:40Z

Compiler doesn't provide any check and inserts _d_assocarrayliteralTX into safe code without a doubt.
It's another one reason to migrate to new AA implementation as possible faster :)

Then please add a comment to the declaration explaining why it is marked as pure even though it isn't.

IgorStepanov · 2014-10-04T13:58:26Z

Then please add a comment to the declaration explaining why it is marked as pure even though it isn't.

done

MartinNowak · 2014-10-04T14:58:10Z

Auto-merge toggled on

MartinNowak · 2014-10-04T14:58:19Z

Thanks a lot for driving this.

9rnsr · 2014-10-04T15:02:37Z

Why aaLiteral is necessary? It looks a thin wrapper and I cannot see the benefit to add it.

9rnsr · 2014-10-04T15:04:57Z

Sorry, ignore my comment.

IgorStepanov · 2014-10-04T15:16:22Z

Thanks! Now it's time to #934
When we done it, we will done the most part of new AA transtion.

[new AA] implement aaLiteral

MartinNowak · 2014-11-24T20:32:47Z

We need to improve this even if it's only an interim interface.
Moving the keys and value to a temporary array is not acceptable, even if it weren't GC allocated.

IgorStepanov · 2014-11-24T20:43:46Z

Moving the keys and value to a temporary array is not acceptable, even if it weren't GC allocated.

How your sugest to do it? Arguments passed in stack in rotation. We need to two separate slices.

if it's only an interim interface.

If we will not linger, we will apply all parts of work and throw _d_assocarrayliteralTX before the next release. End users will not notice this interface.

MartinNowak · 2014-11-24T21:20:48Z

How your sugest to do it?

Indeed it's fairly difficult as long as the implementation sits in rt. So yes we need to defer this.

IgorStepanov mentioned this pull request Aug 22, 2014

Add aaInit and aaLiteral to constructing AA through them #678

Closed

yebblies reviewed Aug 23, 2014
View reviewed changes

IgorStepanov mentioned this pull request Aug 23, 2014

[new AA] use aaLiteral and aaInit dlang/dmd#3892

Closed

IgorStepanov force-pushed the newaa-aaLiteral-and-aaInit branch 4 times, most recently from 8396e88 to 2c03e7a Compare August 26, 2014 19:37

IgorStepanov changed the title ~~[new AA] implement aaLiteral and aaInit~~ [new AA] implement aaLiteral Aug 26, 2014

IgorStepanov mentioned this pull request Aug 26, 2014

[new AA] aaLiteral support dlang/dmd#3904

Closed

dnadlinger reviewed Sep 19, 2014
View reviewed changes

IgorStepanov mentioned this pull request Sep 20, 2014

[new AA] implement core.internal.aa #934

Closed

MartinNowak reviewed Sep 20, 2014
View reviewed changes

IgorStepanov force-pushed the newaa-aaLiteral-and-aaInit branch 2 times, most recently from a36d857 to ea68760 Compare September 21, 2014 01:43

IgorStepanov force-pushed the newaa-aaLiteral-and-aaInit branch from ea68760 to 87c1ca8 Compare September 22, 2014 16:22

MartinNowak reviewed Oct 1, 2014
View reviewed changes

IgorStepanov force-pushed the newaa-aaLiteral-and-aaInit branch from 87c1ca8 to acb4399 Compare October 1, 2014 09:41

MartinNowak reviewed Oct 4, 2014
View reviewed changes

IgorStepanov force-pushed the newaa-aaLiteral-and-aaInit branch from acb4399 to d236d1c Compare October 4, 2014 10:24

IgorStepanov force-pushed the newaa-aaLiteral-and-aaInit branch from 7c37851 to 7c52734 Compare October 4, 2014 11:23

implement aaLiteral

a213c6b

IgorStepanov force-pushed the newaa-aaLiteral-and-aaInit branch from 7c52734 to a213c6b Compare October 4, 2014 13:57

MartinNowak added a commit that referenced this pull request Oct 4, 2014

Merge pull request #933 from IgorStepanov/newaa-aaLiteral-and-aaInit

7e2a075

[new AA] implement aaLiteral

MartinNowak merged commit 7e2a075 into dlang:master Oct 4, 2014

MartinNowak mentioned this pull request Nov 26, 2014

change API for templated aaLiteral #1046

Merged

[new AA] implement aaLiteral #933

[new AA] implement aaLiteral #933

Conversation

IgorStepanov commented Aug 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorStepanov commented Aug 26, 2014

IgorStepanov commented Sep 2, 2014

quickfur commented Sep 16, 2014

IgorStepanov commented Sep 16, 2014

quickfur commented Sep 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorStepanov commented Sep 21, 2014

IgorStepanov commented Sep 22, 2014

IgorStepanov commented Sep 23, 2014

IgorStepanov commented Sep 29, 2014

IgorStepanov commented Sep 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinNowak commented Oct 1, 2014

IgorStepanov commented Oct 1, 2014

IgorStepanov commented Oct 3, 2014

MartinNowak commented Oct 4, 2014

MartinNowak commented Oct 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorStepanov commented Oct 4, 2014

IgorStepanov commented Oct 4, 2014

MartinNowak commented Oct 4, 2014

MartinNowak commented Oct 4, 2014

IgorStepanov commented Oct 4, 2014

IgorStepanov commented Oct 4, 2014

MartinNowak commented Oct 4, 2014

IgorStepanov commented Oct 4, 2014

MartinNowak commented Oct 4, 2014

MartinNowak commented Oct 4, 2014

9rnsr commented Oct 4, 2014

9rnsr commented Oct 4, 2014

IgorStepanov commented Oct 4, 2014

MartinNowak commented Nov 24, 2014

IgorStepanov commented Nov 24, 2014

MartinNowak commented Nov 24, 2014