Skip to content
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.

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

add argument 'capacity~' for @hashset.new #1477

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Conversation

myfreess
Copy link
Contributor

No description provided.

Copy link

peter-jerry-ye-code-review bot commented Jan 14, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Here are three observations from the provided git diff output:

  1. Inconsistent Default Capacity Usage:

    • In the hashset.mbt file, the new function now accepts an optional capacity parameter with a default value of default_init_capacity. However, the entries field is still initialized with default_init_capacity instead of using the provided capacity parameter. This could lead to inconsistencies if the user provides a custom capacity. The entries field should also use the capacity parameter to ensure the internal array matches the specified capacity.
    - entries: FixedArray::make(default_init_capacity, None),
    + entries: FixedArray::make(capacity, None),
  2. Incomplete Default Value in Interface:

    • In the hashset.mbti file, the new function's capacity parameter is documented as capacity~ : Int = ... The .. notation is unclear and might be a placeholder. It should explicitly specify the default value (default_init_capacity) to match the implementation in hashset.mbt.
    - new[K](capacity~ : Int = ..) -> Self[K]
    + new[K](capacity~ : Int = default_init_capacity) -> Self[K]
  3. Potential Misalignment in growAt Calculation:

    • The growAt field is calculated using calc_grow_threshold(default_init_capacity), but it should likely use the provided capacity parameter instead. This ensures that the growth threshold is correctly calculated based on the actual capacity of the hash set.
    - growAt: calc_grow_threshold(default_init_capacity),
    + growAt: calc_grow_threshold(capacity),

These changes would ensure consistency and correctness in the implementation and interface of the hash set.

@coveralls
Copy link
Collaborator

coveralls commented Jan 14, 2025

Pull Request Test Coverage Report for Build 4758

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 83.054%

Totals Coverage Status
Change from base Build 4756: 0.0%
Covered Lines: 4857
Relevant Lines: 5848

💛 - Coveralls

@bobzhang bobzhang force-pushed the myfreess/modify-hashset-new branch from 7925a06 to 19ebaa5 Compare January 14, 2025 05:41
@bobzhang bobzhang enabled auto-merge (rebase) January 14, 2025 05:41
@bobzhang bobzhang merged commit da70491 into main Jan 14, 2025
14 checks passed
@bobzhang bobzhang deleted the myfreess/modify-hashset-new branch January 14, 2025 05:44
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.

3 participants