This adds a require and update function for Associative Arrays #2162

GilesBathgate · 2018-04-14T16:30:04Z

The require function provides a means to get a value corresponding to the key, but if the value doesn't exist it will evaluate the lazy argument to create a new value, add this to the associative array and then return it.

auto p = lookup.require("giles", new Person);
p.eyeColor = Color.Brown;
//...
assert("giles" in lookup);

The update function provides a means to perform additional operations during an aa create or update.

Person newer;
Person older;
lookup.update("giles", {
    newer = new Person; // assign a new value
    return newer;
}, (ref Person p) {
    older = p; // read the old value
    newer = new Person; // assign a new value
    return newer;
});

Corresponding documentation added in dlang/dlang.org#2343

Implementation notes

Traditionally the first example could be done like so:

auto p = "giles" in lookup;
if (p is null) {
    p = new Person;
    lookup["giles"] = p;
}
p.eyeColor = Color.Brown;
//...
assert("giles" in lookup);

I find this later code clunky and it requires two hashes/lookups. The proposed implementation adds support directly to rt/aaA.d to avoid this. I should also point out that the allocation of a new Person in the example is trivial, but it might often be the case that the person instance is created by fetching a record from a db, or an Image loaded from disk, or a movie downloaded from the internet.

Furthermore, the second example could be done like so:

Person newer;
Person older;
auto p = "giles" in lookup;
if (p is null) {
    newer = new Person;
    lookup["giles"] = newer;
} else {
    older = *p;
    newer = new Person;
    *p = newer;
}

While the this isn't any more clunky it requires two hashes/lookups.

Other languages

Several other languages have support for these operations:
C# has GetOrAdd, AddOrUpdate
https://msdn.microsoft.com/en-us/library/ee378677(v=vs.110).aspx
https://msdn.microsoft.com/en-us/library/ee378675(v=vs.110).aspx

Java has computeIfAbsent
https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#computeIfAbsent-K-java.util.function.Function-

Python has setdefault
https://docs.python.org/2/library/stdtypes.html#dict.setdefault

dlang-bot · 2018-04-14T16:30:06Z

Thanks for your pull request and interest in making D better, @GilesBathgate! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2162"

DmitryOlshansky · 2018-04-15T21:09:14Z

Save for the name the idea is awesome and it was a blocker for eg use of builtin AAs in DMD. I never seen add mean insert in D language except maybe in my codepoint set stuff in std.uni.

Bikesheding the name can be elsewhere though;)

GilesBathgate · 2018-04-15T21:21:11Z

@DmitryOlshansky I don't really like the name getOrAdd either ;) (EDIT: now updated to require) But I wanted something terse that would fit with the existing get. Here are a few alternatives getOrInsert upsert merge emplace upget getsert introduce getOrCreate
Where is an appropriate place to bikeshed the name?

wilzbach · 2018-04-15T22:34:12Z

The NG (aka forum.dlang.org - general)

GilesBathgate · 2018-04-15T22:57:34Z

@wilzbach @DmitryOlshansky : ok we can discuss here then: https://forum.dlang.org/post/ejdiweskylrprbculbna@forum.dlang.org

aG0aep6G · 2018-04-17T12:29:24Z

src/object.d

+    assert("bar" !in aa);
+}
+
+auto getOrAdd(K, V)(ref V[K] aa, K key, lazy V value)


Return by ref, please. Could also use V.init as a default for value. Then we can write code like this:

struct S { int value; } S[string] aa; aa.getOrAdd("foo").value = 42; assert(aa == ["foo": S(42)]);

I wondered whether it should be ref (which is why I added the basic value unit test), I hadn't considered this scenario, I will add a test for it and update.

The default value is also a nice idea, so I will add that too (I didn't know lazy args could have a default value)

Hm... I just suggested a name of getPtr on the newsgroup, as I assumed that's what you were doing, but it seems you actually are returning by value. I think returning by ref is preferable to either (maybe getRef?). ~~Note that if you wanted to create a pointer to the value, you have to go through a triple lookup today~~ (Edit: it's not necessary to do it this way, but not as obvious, see my comment below):

Value *p = void; if(!(p = key in aa)) // one { aa[key] = initializeValue(); // two p = key in aa; // three }

Having this function return by ref makes this very easy to do in one lookup.

Note that a class value type is going to be confusing here unless you do a default value of new Value.

@schveiguy getPtrand getRef are certainly both terse which I like. My only criticism is that they don't as clearly communicate that the value will be added/inserted into the associative array.

I am a little bit confused by the triple lookup example. Can I not just do p = (aa[key] = initializeValue())

That said this PR will make this much more simple: Value* p = aa.getOrAdd(key, initializeValue()); Is there a unittest you can suggest to ensure this works as you expected?

Can I not just do p = (aa[key] = initializeValue())

You are still thinking in terms of classes. You may want a pointer to a value type (think int[int]), where you need to use key in aa to avoid further lookups.

Hm... I guess you can do p = &(aa[key] = initializeValue()), which I didn't realize is possible. So no, it's not necessarily a triple lookup.

they don't as clearly communicate that the value will be added/inserted into the associative array

A fair criticism. My response would be, it's just something people will learn. If you are getting a reference to the AA value, it needs to be in there.

In all honesty, before looking closer, I thought the get function already did this.

In all honesty, before looking closer, I thought the get function already did this.

Ha, yeah I made the same mistake...that lead to a few hours of head scratching ;)

@schveiguy I have added a unittest which I think covers your example. @aG0aep6G I've made the updates as you've suggested too.

GilesBathgate · 2018-04-17T17:01:59Z

src/object.d

+    assert(b == S(2));
+
+    S* c = &aa.getOrAdd("baz", S(4));
+    assert(c is &aa["baz"]);


@schveiguy This is the unittest I am referring to.

schveiguy · 2018-04-17T19:05:32Z

src/object.d

+
+unittest
+{
+    struct S


static struct here, otherwise there is a connection between the unittest stack frame and every S instance.

I believe the requested changes were made

JinShil · 2018-06-05T01:00:42Z

I do think this could benefit from a changelog entry, however. It's quite an important change, I think it would be good to advertise it's availability. See https://github.com/dlang/druntime/blob/master/changelog/README.md

JinShil · 2018-06-08T22:30:04Z

@GilesBathgate I'm just waiting on a changelog entry.

JinShil · 2018-06-19T02:53:03Z

Disabled auto-merge so I can manually squash commits when all tests pass.

Geod24

Nice!

Geod24 · 2018-06-19T03:23:12Z

src/object.d

+ *      update = The delegate to apply on update.
+ */
+void update(K, V, C, U)(ref V[K] aa, K key, C create, U update)
+if ((is(C : V delegate()) || is(C : V function())) && (is(U : V delegate(ref V)) || is(U : V function(ref V))))


I'm not sure this is the right approach. It forbids opCall and restrict some conversion which would be legal under the typesystem.
Maybe use is(typeof({ V* ptr; *ptr = create(); }) ?
Also shouldn't the arguments create and update be scope to avoid pointless allocation when passing delegate literals ?

Geod24 · 2018-06-19T03:23:31Z

src/object.d

+
+unittest
+{
+    static class C{}


Nit: add a space

@Geod24 Between C and {}?

Geod24 · 2018-06-19T03:26:23Z

src/object.d

+ * Returns:
+ *      The value.
+ */
+ref V require(K, V)(ref V[K] aa, K key, lazy V value = V.init) pure


I would remove the pure annotation and make the template works its magic, or is there a specific reason you put it ?

@Geod24 I Just copied the function attributes from the get function.

@Geod24 Just so I am clear this is the 'magic' you are referring to: http://klickverbot.at/blog/2012/05/purity-in-d/#templates-and-purity i.e templates infer pure?
Presumably this wasn't the case when 'get' was implemented.

Template infers attributes, so @nogc, @safe/@system, and pure
In this case since only pure and nothrow are put on the extern (C) declaration, that's as much as could get infered. Maybe there were some inference bug at the time ?

You can make sure this works by making a pure unittest

JinShil · 2018-06-19T03:35:25Z

I'll refrain from merging this until @Geod24's comments are addressed.

JinShil · 2018-06-20T23:28:25Z

@GilesBathgate Nice Work! And, thank you for sticking with it.

@Geod24 Could you please give this one last look. I'm itching to merge this.

Geod24

Looks very good to me, thanks!

JinShil · 2018-06-21T02:52:52Z

Disabled auto-merge so I can manually squash commits when all tests pass.

CyberShadow · 2020-03-31T05:11:29Z

I'm proposing a small modification (addition) to the API:
#3012

Thank you @GilesBathgate for originally adding these, I got a lot of use out of them! :)

GilesBathgate requested review from andralex and MartinNowak as code owners April 14, 2018 16:30

GilesBathgate mentioned this pull request Apr 14, 2018

Add documentation for require function to associative arrays dlang/dlang.org#2343

Merged

GilesBathgate requested a review from wilzbach as a code owner April 14, 2018 19:24

GilesBathgate force-pushed the getOrAdd branch 2 times, most recently from 62fbe02 to d1aed77 Compare April 15, 2018 19:17

aG0aep6G reviewed Apr 17, 2018

View reviewed changes

GilesBathgate force-pushed the getOrAdd branch from d1aed77 to d1aa3fc Compare April 17, 2018 16:59

GilesBathgate commented Apr 17, 2018

View reviewed changes

GilesBathgate force-pushed the getOrAdd branch 2 times, most recently from 9373b0b to 3843de8 Compare April 17, 2018 18:38

schveiguy reviewed Apr 17, 2018

View reviewed changes

GilesBathgate force-pushed the getOrAdd branch 2 times, most recently from e08c689 to 1c7e2f0 Compare April 19, 2018 16:56

GilesBathgate changed the title ~~This adds an Associative Array function getOrAdd~~ This adds a require function for Associative Arrays Apr 19, 2018

GilesBathgate force-pushed the getOrAdd branch 6 times, most recently from 50efe1a to 6a8b06f Compare April 20, 2018 11:20

GilesBathgate changed the title ~~This adds a require function for Associative Arrays~~ This adds a require and update function for Associative Arrays Apr 20, 2018

GilesBathgate force-pushed the getOrAdd branch 3 times, most recently from df11c74 to 1ac1702 Compare April 20, 2018 17:24

JinShil removed the @andralex Approval from Andrei is required label Jun 5, 2018

JinShil unassigned andralex Jun 5, 2018

JinShil approved these changes Jun 5, 2018

View reviewed changes

Add changelog entry

13b6361

GilesBathgate force-pushed the getOrAdd branch from 2a50ad1 to 13b6361 Compare June 16, 2018 12:50

JinShil added 72h no objection -> merge The PR will be merged if there are no objections raised. auto-merge and removed auto-merge labels Jun 16, 2018

Geod24 approved these changes Jun 19, 2018

View reviewed changes

JinShil removed the 72h no objection -> merge The PR will be merged if there are no objections raised. label Jun 19, 2018

Ensure pure is inferred by template

15a9bb6

GilesBathgate force-pushed the getOrAdd branch from 398d292 to 15a9bb6 Compare June 19, 2018 17:09

GilesBathgate added 2 commits June 19, 2018 19:36

Add scope to parameters, fix whitespace

c3706d4

Add support for opCall

0648fac

GilesBathgate force-pushed the getOrAdd branch from 021c9e5 to 0648fac Compare June 20, 2018 20:21

Geod24 approved these changes Jun 21, 2018

View reviewed changes

JinShil added auto-merge and removed auto-merge labels Jun 21, 2018

JinShil removed the auto-merge label Jun 21, 2018

JinShil merged commit 0c92d13 into dlang:master Jun 21, 2018

GilesBathgate deleted the getOrAdd branch June 23, 2018 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

This adds a require and update function for Associative Arrays #2162

This adds a require and update function for Associative Arrays #2162

GilesBathgate commented Apr 14, 2018 •

edited

Loading

dlang-bot commented Apr 14, 2018

DmitryOlshansky commented Apr 15, 2018

GilesBathgate commented Apr 15, 2018 •

edited

Loading

wilzbach commented Apr 15, 2018

GilesBathgate commented Apr 15, 2018

aG0aep6G Apr 17, 2018

GilesBathgate Apr 17, 2018

GilesBathgate Apr 17, 2018

schveiguy Apr 17, 2018 •

edited

Loading

GilesBathgate Apr 17, 2018

schveiguy Apr 17, 2018

schveiguy Apr 17, 2018

schveiguy Apr 17, 2018

GilesBathgate Apr 17, 2018 •

edited

Loading

GilesBathgate Apr 17, 2018

GilesBathgate Apr 17, 2018

schveiguy Apr 17, 2018

JinShil commented Jun 5, 2018

JinShil commented Jun 8, 2018

JinShil commented Jun 19, 2018

Geod24 left a comment

Geod24 Jun 19, 2018

Geod24 Jun 19, 2018

GilesBathgate Jun 19, 2018

Geod24 Jun 19, 2018

Geod24 Jun 19, 2018

GilesBathgate Jun 19, 2018

GilesBathgate Jun 19, 2018

Geod24 Jun 19, 2018

JinShil commented Jun 19, 2018

JinShil commented Jun 20, 2018

Geod24 left a comment

JinShil commented Jun 21, 2018

CyberShadow commented Mar 31, 2020

This adds a require and update function for Associative Arrays #2162

This adds a require and update function for Associative Arrays #2162

Conversation

GilesBathgate commented Apr 14, 2018 • edited Loading

dlang-bot commented Apr 14, 2018

Bugzilla references

Testing this PR locally

DmitryOlshansky commented Apr 15, 2018

GilesBathgate commented Apr 15, 2018 • edited Loading

wilzbach commented Apr 15, 2018

GilesBathgate commented Apr 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schveiguy Apr 17, 2018 • edited Loading

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

GilesBathgate Apr 17, 2018 • edited Loading

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

JinShil commented Jun 5, 2018

JinShil commented Jun 8, 2018

JinShil commented Jun 19, 2018

Geod24 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil commented Jun 19, 2018

JinShil commented Jun 20, 2018

Geod24 left a comment

Choose a reason for hiding this comment

JinShil commented Jun 21, 2018

CyberShadow commented Mar 31, 2020

GilesBathgate commented Apr 14, 2018 •

edited

Loading

GilesBathgate commented Apr 15, 2018 •

edited

Loading

schveiguy Apr 17, 2018 •

edited

Loading

GilesBathgate Apr 17, 2018 •

edited

Loading