8000 use String::make to construct large strings in test by FlyCloudC · Pull Request #2378 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
/ core Public
< 8000 /div>

use String::make to construct large strings in test #2378

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
Merged
merged 1 commit into from
Jun 29, 2025
Merged

use String::make to construct large strings in test #2378

merged 1 commit into from
Jun 29, 2025

Conversation

FlyCloudC
Copy link
Contributor

No description provided.

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
Inconsistent test data between string generation and expected content

Category
Correctness
Code Snippet
let s = String::make(1000, '9')
inspect(parse_decimal_priv(s), content=String::make(800, '9'))
Recommendation
Use consistent character and length: let s = "1" + String::make(999, '0') to match the original intent of testing "1" followed by zeros, or ensure the expected content matches the input
Reasoning
The original test created a string starting with '1' followed by '0's, but the refactored version creates all '9's. Additionally, the input is 1000 '9's but expected content is only 800 '9's, which suggests potential truncation behavior that should be explicitly documented or corrected

Magic numbers without explanation in test

Category
Maintainability
Code Snippet
let s = String::make(1000, '9')
inspect(parse_ 8000 decimal_priv(s), content=String::make(800, '9'))
Recommendation
Add constants or comments explaining the significance: const LARGE_INPUT_SIZE = 1000 const TRUNCATED_OUTPUT_SIZE = 800 // Test truncation behavior when input exceeds internal precision limits
Reasoning
The numbers 1000 and 800 appear to test specific truncation behavior but lack context. Adding named constants or comments would make the test's purpose clearer and easier to maintain

Efficient string generation replaces inefficient loop concatenation

Category
Performance
Code Snippet
let s = String::make(100, '9') // Instead of:
// let mut s = ""
// for i in 0..<100 {
// s += "9"
// }
Recommendation
Keep the current implementation using String::make()
Reasoning
The refactored code correctly eliminates O(n²) string concatenation in favor of O(n) direct allocation, which is much more efficient for generating large strings

@coveralls
Copy link
Collaborator
coveralls commented Jun 29, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 149

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 89.386%
Totals Coverage Status
Change from base Build 146: 0.03%
Covered Lines: 3453
Relevant Lines: 3863
💛 - Coveralls

@bobzhang bobzhang enabled auto-merge (rebase) June 29, 2025 23:18
@bobzhang bobzhang merged commit b6b1fc9 into moonbitlang:main Jun 29, 2025
10 checks passed
@FlyCloudC FlyCloudC deleted the avoid-string-add branch July 2, 2025 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FlyCloudC @coveralls @bobzhang
0