Skip to content

fix(core): qualify dflt-len in Dynamic.Map.shrink - #1607

Open
bercedev wants to merge 4 commits into
carp-lang:masterfrom
bercedev:fix/dynamic-map-shrink
Open

bercedev wants to merge 4 commits into
carp-lang:masterfrom
bercedev:fix/dynamic-map-shrink

Conversation

@bercedev

Copy link
Copy Markdown
Contributor

Fixes #1600.

went with what you found in the comment: shrink used dflt-len unqualified, and a defndynamic resolves from the root, so it never saw the dynamic one in the Map module. it's Map.dflt-len now, same as the other funs use Map.min-load.

added two tests to test/dynamic_map.carp: shrinking a fresh map leaves it at 16 buckets, and shrinking a 64 bucket map gives 32. on master that file now stops with Can't find symbol 'dflt-len' at core/Map.carp:711:25, with the fix all 37 tests in it pass.

i ran the other test/*.carp files too, same results as master apart from the new tests (init_global, managed, memory and unsafe don't pass in my container on master either, so i left those alone).

A defndynamic resolves names from the root, not its module, so the unqualified dflt-len in shrink was not found. Use Map.dflt-len, as the other dynamic Map functions do with Map.min-load. Fixes carp-lang#1600.
@hellerve

Copy link
Copy Markdown
Member

Hi Berce, and thanks for contributing. Just one question: what was the role of AI assistance in this patch? We’re not against AI-assisted development, but we ask that people are transparent about it. Also, especially on issues marked "beginner", we’re mostly interested in people who want to start engaging with the project rather than drive-by contributions.

@bercedev

Copy link
Copy Markdown
Contributor Author

hi, honest answer: the code was written by claude code, and i tested it locally before opening the PR (built carp with gcc and ran the test suite, including the new tests). totally get the point about beginner issues, if you'd rather keep those for people starting out with the project i'm fine closing this one

@hellerve

Copy link
Copy Markdown
Member

thanks for your candor! was this meant more of a drive-by, or did you want to enter the community? if it’s the former no hard feelings at all, but i think we should close this.

@bercedev

Copy link
Copy Markdown
Contributor Author

not meant as a drive-by, i'd like to stick around and keep contributing, mostly fixes through PRs like this one. if that works for you i'm happy to keep this and #1608 open and work through any review

@hellerve

Copy link
Copy Markdown
Member

works for me! feel free to join the discord, too, since that’s where the chatter happens :)

@hellerve hellerve left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct, but it doesn’t fix #1600 completely (the remove and put bugs are unfixed).

put grew once the load passed min-load instead of max-load. remove shrank while the load was above min-load and only filtered the bucket afterwards, rebuilding the bucket list in reverse, so the other keys could land in the wrong buckets. It now drops the key from its bucket and shrinks one step when the load falls below min-load, like the static Map.
@bercedev

Copy link
Copy Markdown
Contributor Author

good catch, pushed fixes for both: put now grows past max-load, and remove drops the key from its own bucket first and then shrinks one step when the load is below min-load and the map is bigger than dflt-len (same as the static shrink!). while in there i found the old remove also rebuilt the bucket list with reduce/cons, which reversed it, so after a remove other keys could end up in the wrong buckets. added tests for the three cases, they fail on master. all of test/*.carp passes locally (77).

Comment thread core/Map.carp Outdated
(if (and (< (/ (* (Map.len removed) 100) (length removed)) Map.min-load)
(> (length removed) Map.dflt-len))
(Map.shrink removed)
removed)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc says a missing key returns the map unchanged, but we now check the siz and shrink first.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, fixed in cf5338c: remove now returns the map as is when the key isn't there, so the doc holds again. added a test for that (a sparse 64-slot map keeps its size when you remove a missing key). full run_carp_tests.sh passes.

Comment thread core/Map.carp Outdated
(let [idx (Dynamic.imod (hash k) (length m))
removed (List.update-nth m idx (fn [l] (filter (fn [pair] (/= (car pair) k)) l)))]
(if (and (< (/ (* (Map.len removed) 100) (length removed)) Map.min-load)
(> (length removed) Map.dflt-len))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(> (length removed) Map.dflt-len) is already in shrink. calling shrink on thefirst condition is enough.

@bercedev

Copy link
Copy Markdown
Contributor Author

right, dropped it. remove doesn't recurse anymore, so shrink returning the map unchanged at the default size is enough. tests still pass (test/*.carp 77, error tests 50).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dynamic.Map.shrink refers to static dflt-len

2 participants