Skip to content

extract small_map as separate utility crate #4214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

extract small_map as separate utility crate #4214

wants to merge 18 commits into from

Conversation

radudiaconu0
Copy link

@radudiaconu0 radudiaconu0 commented Mar 22, 2025 •
edited
Loading

This Pull Request closes #4184 .

It changes the following:

  • extract small_map as separate utility crate

@radudiaconu0
Copy link
Author

@jedel1043

@radudiaconu0
Copy link
Author

radudiaconu0 commented Mar 22, 2025 •
edited
Loading

@jedel1043 I think I did something wrong

@radudiaconu0
Copy link
Author

@jedel1043 i added docs here

@codecov Codecov
Copy link

codecov bot commented Mar 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.59%. Comparing base (6ddc2b4) to head (6ab4ae3).
Report is 396 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4214      +/-   ##
==========================================
+ Coverage   47.24%   52.59%   +5.34%     
==========================================
  Files         476      487      +11     
  Lines       46892    51965    +5073     
==========================================
+ Hits        22154    27330    +5176     
+ Misses      24738    24635     -103

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nekevss nekevss added the Internal Category for changelog label Mar 24, 2025
@nekevss nekevss requested a review from a team March 24, 2025 13:09
Razican
Razican approved these changes Mar 27, 2025
View reviewed changes
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

I propose minor gramatical / style enhancements that don't prevent the merge from my side.

radudiaconu0 and others added 14 commits March 27, 2025 11:06
@radudiaconu0 @Razican
Update ABOUT.md
b99e6a3 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update utils/tag_ptr/ABOUT.md
91dd81c 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update utils/small_map/Cargo.toml
e27e71b 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update utils/small_map/ABOUT.md
d8e9069 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update cli/ABOUT.md
0c190db 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update core/ast/ABOUT.md
bba0272 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update core/engine/ABOUT.md
ac0ea5d 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update core/string/ABOUT.md
bc5d29c 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update core/runtime/ABOUT.md
f967c48 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update core/gc/ABOUT.md
e3ad8a2 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update core/icu_provider/ABOUT.md
c3db0df 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update core/interner/ABOUT.md
bb0cccb 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update core/parser/ABOUT.md
89b998c 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0 @Razican
Update core/macros/ABOUT.md
10a2f7a 
Co-authored-by: Iban Eguia Moraza <[email protected]>
@radudiaconu0
Copy link
Author

@Razican i commited your suggestions

@oriongonza
Copy link

oriongonza commented Mar 27, 2025 •
edited
Loading

your git history doesn't look right anymore, can you run git rebase -i on it?

nekevss
nekevss reviewed Mar 27, 2025
View reviewed changes
Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

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

This is looking great! Thanks for working on this! It's really appreciated 😄

Had a couple points that I noticed when reviewing.

nekevss
nekevss reviewed Mar 27, 2025
View reviewed changes
utils/small_map/src/entry.rs
@@ -219,6 +224,7 @@ impl<'a, K: Ord, V, const ARRAY_SIZE: usize> OccupiedEntry<'a, K, V, ARRAY_SIZE>

/// Sets the value of the entry with the `OccupiedEntry`'s key,
/// and returns the entry's old value.
#[must_use]
Copy link
Member

@nekevss nekevss Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Huh, actually after seeing the broken lints, I'm wondering if both insert and remove should have a must_use attribute. It might be a bit too strict

While the failure's aren't exactly incorrect, it feels like a negative of the API to force users to use let _ = map.insert(value) vs. map.insert(value)

HalidOdat
HalidOdat reviewed Mar 29, 2025
View reviewed changes
utils/small_map/Cargo.toml
keywords = ["small", "map"]
categories = ["data-structures"]
readme = "../../README.md"
description = "Utility library that add SmallMap data structure"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe describe the data-structure instead, something like SmallMap is an inline vec that grows into a heap map past ARRAY_SIZE

utils/small_map/Cargo.toml
@@ -0,0 +1,22 @@
[package]
name = "small_map"
Copy link
Member

Choose a reason for hiding this comment

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

Both small_map and smallmap are taken names. Maybe we should think of a different name? any ideas @boa-dev/maintainers ?

Copy link
Member

Choose a reason for hiding this comment

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

small_btree?

@HalidOdat HalidOdat added this to the next-release milestone Mar 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HalidOdat HalidOdat HalidOdat left review comments

@jedel1043 jedel1043 jedel1043 left review comments

@nekevss nekevss nekevss left review comments

@Razican Razican Razican approved these changes

Assignees
No one assigned
Labels
Internal Category for changelog technical debt
Projects
None yet
Milestone
next-release
Development

Successfully merging this pull request may close these issues.

Extract SmallMap into an utility crate
6 participants
@radudiaconu0 @oriongonza @Razican @HalidOdat @jedel1043 @nekevss