Small hashmap additions #1709

Jason94 · 2025-12-08T04:55:35Z

Adds two small functions that just wrap update, a docstring, and an (Into x String) instance.

@shirok

stylewarning · 2025-12-08T05:47:58Z

will wait for @shirok to comment

shirok

I'm curious how it will fit into the broader plan of mapping and collection interface.

Also, tests would be nice, even the trivial ones.

shirok · 2025-12-08T05:50:24Z

library/hashmap.lisp

      (Tuple (HashMap newnode) aux)))

+  (inline)
+  (declare modify (Hash :k => HashMap :k :v -> :k -> (:v -> :v) -> Tuple (HashMap :k :v) (Optional :v)))


It looks like the code is written to return Tuple (Optional (HashMap :k :v)) (Optional :v).

If this pattern is useful, we want to have it in the mapping protocol. I wonder, though, if it is useful to return (Optional (HashMap :k :v))---as update is the most general protocol, we want modify to be a convenience utility, as easy to use as possible. Do the caller wants to know whether the map is modified or not?

It does compile, if that's what you're concerned about. The overall modify function just returns the HashMap, and returns an optional modified value (None in case the the key was missing). I included this version that returns the (Optional :v) because sometimes it is nice to do a modify-and-get all at once. But I could be convinced to get rid of this one, and just make modify_ be modify. I'm sure that's the most common of any of these "specialized version of update" use cases.

Oh, and feel free to chime in here: #1329. It would be great to get all of the good discussion on the map interface together.

Ah, my bad. I confused update protocol even I wrote that. Please disregard Optional thing.

shirok · 2025-12-08T05:52:18Z

library/hashmap.lisp

+                          (Tuple (Some result) (Some result))))))))
+
+  (inline)
+  (declare modify_ (Hash :k => HashMap :k :v -> :k -> (:v -> :v) -> HashMap :k :v))


Same as above: The code returns (Optional (HashMap :k :v)).

Another question is the naming convention. If we'll have modify and modify_, I'd like to have the (mostly) consistent interface for other foo and foo_ variations. If this variations fit into the broader collection framework, I'm ok with it.

Are you sure? It's compiling on my machine, and I'm using it in a project. Maybe our versions of the repo are out of sync, or something? In any case, I just added some tests, so maybe take a look at those?

Yeah, the _ suffix convention is something I like using for simpler versions of functions that have a less expressive (but sometimes more useful) type signature. I don't know if there's much of that pattern in the standard library, though. Alternatively, this one could be modify and the other one could be modify-get or something.

As long as the convention of _ will be established, I'm fine with it.

shirok · 2025-12-08T05:56:44Z

library/hashmap.lisp

  (define-instance (Hash :k => Monoid (HashMap :k :v))
    (define mempty Empty))
+
+  (define-instance ((Hash :k) (Into :k String) (Into :v String) => Into (HashMap :k :v) String)


Into * String can be one of two things: Convert a thing into a string representation (obtaining external representation), or convert a sequence to a character sequence. Example of the latter is to convert a list of characters or a vector of characters into a string. I think those two needs to have distinct names.

My instinct is to keep Into * String as a sequence-seqence conversion (e.g. so it can be parallel with Into * List or Into * Seq etc., and have a separate protocol to get an external representation, much like Haskell's show.

Yeah, I agree about Show. I think that's the long term plan. (#1641). And I agree this would be more suited to a "Show" instance than a conversion instance.

We could put this in for now and change it to Show later, when it gets added, or wait to add this (or something like it) then.

I'm for having intermediate plan before we have full show protocol. In that case, how about having a different name (e.g. to-string or stringify) so that it will be easier to move once show is in place?

I'm afraid that once we conflate into, it would be cumbersome to discern different protocols later.

I agree with this. Maybe even just call it show-*.

Just pushed up a commit simply calling it show. Eventually when we do get a show typeclass, that should make it easy for any code that uses it to convert.

stylewarning · 2025-12-08T22:23:05Z

library/hashmap.lisp

   #:remove
   #:update
+   #:modify
+   #:modify_


I'm not a big fan of this convention. It's vague ("the simpler/reduced call"). I don't feel the need to be a stickler about it though.

stylewarning · 2025-12-08T22:25:09Z

@shirok I reset your review since you just commented.

I'm not a huge fan of the _ convention, but I'm happy to make it one if we can "define" it sufficiently well. I want the more useful functions to generally be the ones with more normal names. Is there something we can agree on here that aligns with that? I almost think modify should just return the map (if I were to "guess" the type of such a function).

I'm open to ideas.

shirok · 2025-12-08T22:42:21Z

@stylewarning Yes, my hunch is it's more natrual to expect modify :: HashMap :k :v -> :k -> (:v -> :v) -> HashMap :k :v. A variation may be to take the "default" value when the key doesn't exist. With the default value, it is easy to write the typical patterns such as "push a value into the entry--if the entry didn't exist, assume ()" or "increment the associated value--if the entry didn't exist, assume 0". In CL, I'd certainly add &optional default.

I don't see much use case in obtaining the new value; if I can spend some more cycles I can lookup again with the key, or I can fall back to the general update. But if it makes some common patterns easier to write, I don't oppose to have it, but with more descriptive name.

Jason94 · 2025-12-08T23:42:07Z

@shirok I reset your review since you just commented.

I'm not a huge fan of the _ convention, but I'm happy to make it one if we can "define" it sufficiently well. I want the more useful functions to generally be the ones with more normal names. Is there something we can agree on here that aligns with that? I almost think modify should just return the map (if I were to "guess" the type of such a function).

I'm open to ideas.

That's reasonable. In that case, I think modify and modify-get would be fine.

stylewarning · 2025-12-10T22:22:40Z

(tests fail because of a bug that's been fixed on main; just rebase)

Adds two small functions, a docstring, and an (Into x String) instance

modify_ -> modify, modify->modify-get

Jason94 · 2025-12-11T02:29:31Z

(tests fail because of a bug that's been fixed on main; just rebase)

Hmm. I rebased but I'm still getting the SBCL failures. It doesn't look like my branch is behind main.

stylewarning · 2025-12-11T02:42:38Z

lol, this is the outdated quicklisp issue and named-readtables...

will fix

stylewarning approved these changes Dec 8, 2025

View reviewed changes

stylewarning requested a review from shirok December 8, 2025 05:48

shirok reviewed Dec 8, 2025

View reviewed changes

stylewarning reviewed Dec 8, 2025

View reviewed changes

stylewarning requested a review from shirok December 8, 2025 22:23

Jason94 added 3 commits December 10, 2025 21:08

Small hashmap additions

861fbd4

Adds two small functions, a docstring, and an (Into x String) instance

Add basic tests

5bced95

Remove Into String instance

cce9b0e

Jason94 force-pushed the hashmap-small-additions branch from 450c6ad to 71cf7af Compare December 11, 2025 02:18

Update function names

dfbc664

modify_ -> modify, modify->modify-get

Jason94 force-pushed the hashmap-small-additions branch from 71cf7af to dfbc664 Compare December 11, 2025 02:28

stylewarning approved these changes Dec 11, 2025

View reviewed changes

stylewarning merged commit 7d463c8 into coalton-lang:main Dec 11, 2025
33 of 50 checks passed

Jason94 deleted the hashmap-small-additions branch December 11, 2025 12:35

Small hashmap additions #1709

Small hashmap additions #1709

Uh oh!

Conversation

Jason94 commented Dec 8, 2025

Uh oh!

stylewarning commented Dec 8, 2025

Uh oh!

shirok left a comment • 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!

shirok Dec 8, 2025 • 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!

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!

shirok Dec 8, 2025 • 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!

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!

stylewarning commented Dec 8, 2025

Uh oh!

shirok commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jason94 commented Dec 8, 2025

Uh oh!

stylewarning commented Dec 10, 2025

Uh oh!

Jason94 commented Dec 11, 2025

Uh oh!

stylewarning commented Dec 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shirok left a comment •

edited

Loading

shirok Dec 8, 2025 •

edited

Loading

shirok Dec 8, 2025 •

edited

Loading

shirok commented Dec 8, 2025 •

edited

Loading