Standard library: Association lists by matthewhammer · Pull Request #231 · caffeinelabs/motoko

matthewhammer · 2019-03-11T23:07:03Z

Changelog:

uniform indenting
association lists, as an alternative to tries for finite maps
tries use association lists for their hash-collision buckets

… xxx's

matthewhammer · 2019-03-11T23:09:15Z

Need a review for merging into master. Thanks!

ggreif

I did not review the algorithms themselves.

Maybe there should be some unit tests.

ggreif · 2019-03-11T23:19:47Z

stdlib/examples/produceExchange.as

    end:    Date,
    cost:   Price,
-    tt:     TruckTypeId
+    tt:     TruckKindId


Yep, thanks!

ggreif · 2019-03-11T23:23:22Z

stdlib/Makefile

 	@echo $(DONE)

-$(OUTDIR)/Trie.out: $(OUTDIR) list.as trie.as
+$(OUTDIR)/AssocList.out: $(OUTDIR) list.as assocList.as


$(OUTDIR) should be added as order-only prerequisite. For other targets too.

Suggested change

$(OUTDIR)/AssocList.out: $(OUTDIR) list.as assocList.as

$(OUTDIR)/AssocList.out: list.as assocList.as | $(OUTDIR)

OK, good point.

I fix this once @nomeata and I settle on the other changes to the Makefile; he has another PR open for the strings defined above.

ggreif · 2019-03-11T23:27:57Z

stdlib/assocList.as

+    rec(al)
+  };
+
+  // The key-value pairs of the final list consists of those pairs of


plural

Suggested change

// The key-value pairs of the final list consists of those pairs of

// The key-value pairs of the final list consist of those pairs of

ggreif · 2019-03-11T23:32:30Z

stdlib/setDb.as

@@ -104,7 +104,7 @@ let SetDb = new {
    let r1 = Set.union<Nat>(s1, s2);
    let r2 = Trie.disj<Nat,(),(),()>(s1, s2, natEq, func (_:?(),_:?()):(())=());
    //xxx


comment still needed?

no, this xxx is indeed stale, thanks!

ggreif · 2019-03-11T23:34:30Z

stdlib/trie.as

+	                  makeBin<K,V>(t0, t1)
+	                };
+             case (false, true) {
+	                  assert(false);


Redundant pares? Below too.

crusso · 2019-03-12T10:28:15Z

stdlib/list.as

+	    case (null, _)    { false };
+	    case (_,    null) { false };
+	    case (?(h1,t1), ?(h2,t2)) {
+	           if (eq(h1,h2)) {


why not just eq(h1,h2) and rec(t1,t2)…?

(and is short-circuiting)

Ah, okay. Even better, thanks!

crusso · 2019-03-12T10:34:58Z

stdlib/trie.as

+	      case (?n) {
+	             assertIsBin<K,V>(t);
+	             let bit = getHashBit(k.hash, bitpos);
+	             if (not bit) { rec(n.left,  bitpos+1) }


Simplify to if bit ...<swapped branches>?

crusso

That's a lot of actorscript!

Looks good, but are the switch/cases all deeply indented because of emacs mode?

crusso · 2019-03-12T10:40:33Z

stdlib/assocList.as

+// polymorphic association linked lists between keys and values
+type AssocList<K,V> = List<(K,V)>;
+
+let AssocList = new {


I'd be tempted to abstract AssocList on k_eq... (which is equally unsafe but maybe more convenient). But it's fine as stands. I guess this is a case where we'd like something like generative functors or just existentials.

I agree that generative functors (or existentials) would be really nice here. It's not too painful to pass around these equality functions, but if there were a bunch more operations in the mix, it might get unwieldy after a while, or require some explicit structures, or both.

matthewhammer · 2019-03-12T13:48:02Z

Thanks for the reviews, everyone! I appreciate the feedback, and the questions.

the switch/cases all deeply indented because of emacs mode

Yes. I'll fix that sometime and then re-indent everything automatically. It's too painful to fight with emacs when it chooses how to indent things, I find. If no one strongly objects, I just want to rely on emacs to do the indenting standard for us.

matthewhammer · 2019-03-12T15:39:53Z

Maybe there should be some unit tests.

Yes, there should. But I think we first need to have some way to generate random instances, and choose random operations to perform. So, hashing will help there, as a prerequisite for generating pseudorandom stuff in a deterministic way, entirely within ActorScript. I'll look into that soon.

matthewhammer · 2019-03-12T15:40:11Z

I've addressed the review comments; going to merge now.

## Changelog for motoko-base: Branch: next-moc Commits: [caffeinelabs/motoko-base@b515e237...7daaa0b0](caffeinelabs/motoko-base@b515e23...7daaa0b) * [`d8877676`](caffeinelabs/motoko-base@d887767) Remove Char.from/toWord32, as they are deprecated ([dfinity/motoko-base⁠#231](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/motoko-base/issues/231))

matthewhammer added 4 commits March 8, 2019 12:02

use actorscript-mode for to make indenting standard in stdlib

1252fbf

association lists; use for tries' hash collisions; still missing some…

7d70057

… xxx's

finish association list implementation

19b1610

tries handle hash collisions with association lists

1503376

matthewhammer changed the title ~~Standard library~~ Standard library: Association lists Mar 11, 2019

matthewhammer requested review from crusso and nomeata March 11, 2019 23:08

ggreif approved these changes Mar 11, 2019

View reviewed changes

crusso reviewed Mar 12, 2019

View reviewed changes

crusso approved these changes Mar 12, 2019

View reviewed changes

review comments

f349e36

matthewhammer merged commit 945563f into master Mar 12, 2019

	$(OUTDIR)/AssocList.out: $(OUTDIR) list.as assocList.as
	$(OUTDIR)/AssocList.out: list.as assocList.as \| $(OUTDIR)

	// The key-value pairs of the final list consists of those pairs of
	// The key-value pairs of the final list consist of those pairs of

Conversation

matthewhammer commented Mar 11, 2019

Uh oh!

matthewhammer commented Mar 11, 2019

Uh oh!

ggreif left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewhammer commented Mar 12, 2019

Uh oh!

matthewhammer commented Mar 12, 2019

Uh oh!

matthewhammer commented Mar 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

crusso Mar 12, 2019 •

edited

Loading