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

Refactor u32::max_value() with u32::MAX #3735

Conversation

Acaccia
Copy link
Contributor

@Acaccia Acaccia commented Jun 5, 2023

Description

I changed all occurrences of deprecated u32::max_value().
While I was at it, I changed the same deprecated method for u64 and i32 hiding there.

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated

@CLAassistant
Copy link

CLAassistant commented Jun 5, 2023

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #3735 (b103715) into develop (56493ac) will decrease coverage by 2.18%.
The diff coverage is 80.63%.

@@             Coverage Diff             @@
##           develop    #3735      +/-   ##
===========================================
- Coverage    85.08%   82.90%   -2.18%     
===========================================
  Files          298      302       +4     
  Lines       276674   286537    +9863     
===========================================
+ Hits        235397   237543    +2146     
- Misses       41277    48994    +7717     
Impacted Files Coverage Δ
...larity/src/vm/analysis/arithmetic_checker/tests.rs 98.39% <ø> (ø)
clarity/src/vm/analysis/errors.rs 72.09% <0.00%> (-0.34%) ⬇️
clarity/src/vm/analysis/read_only_checker/tests.rs 100.00% <ø> (ø)
clarity/src/vm/analysis/trait_checker/tests.rs 95.17% <ø> (ø)
.../src/vm/analysis/type_checker/v2_1/tests/assets.rs 100.00% <ø> (ø)
...c/vm/analysis/type_checker/v2_1/tests/contracts.rs 96.39% <ø> (ø)
...ity/src/vm/analysis/type_checker/v2_1/tests/mod.rs 99.08% <ø> (ø)
clarity/src/vm/tests/defines.rs 97.84% <ø> (ø)
clarity/src/vm/tests/sequences.rs 99.60% <ø> (ø)
clarity/src/vm/tests/simple_apply_eval.rs 99.56% <ø> (ø)
... and 73 more

... and 33 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@netrome netrome self-requested a review June 5, 2023 15:20
Copy link
Contributor

@pavitthrap pavitthrap left a comment

Choose a reason for hiding this comment

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

LGTM, just a small nit on the changelog.

CHANGELOG.md Outdated
@@ -21,6 +21,12 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
runtime error and include details about the error in the `vm_error` field of
the receipt. Fixes issues #3154, #3328.

### Changed

- All occurrences of the deprecated method `u32::max_value()` (as well as `u64`
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't the type of change that needs to be recorded in the changelog in my opinion (nothing is changing externally).

Copy link
Contributor Author

@Acaccia Acaccia Jun 6, 2023

Choose a reason for hiding this comment

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

Yeah, I agree. I will remove that entirely.

Copy link
Contributor

@netrome netrome left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@pavitthrap pavitthrap changed the base branch from develop to chore/merge-u32-max June 6, 2023 16:12
@pavitthrap pavitthrap merged commit 4c0182a into stacks-network:chore/merge-u32-max Jun 6, 2023
1 of 2 checks passed
@igorsyl
Copy link
Contributor

igorsyl commented Jun 6, 2023

Congrats @Acaccia on your first merged Stacks blockchain PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants