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

[Framework][Minor] Add _ before unused self variables to avoid compiler warnings #14968

Closed
wants to merge 1 commit into from

Conversation

movekevin
Copy link
Contributor

Description

self is not actually used in pool_u64::multiply_then_divide or pool_u64_unbounded::multiply_then_divide. This is currently generating compiler warnings every time a package is compiled against the framework. This PR adds _ prefix to avoid these warnings.

How Has This Been Tested?

Compilation

Key Areas to Review

Minor changes

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Other (specify)

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link

trunk-io bot commented Oct 15, 2024

⏱️ 10m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-move-tests 4m 🟥
rust-move-unit-coverage 2m 🟥
rust-cargo-deny 2m 🟩
check-dynamic-deps 39s 🟩
general-lints 33s 🟩
semgrep/ci 21s 🟩
file_change_determinator 10s 🟩
Backport PR 4s 🟥
permission-check 4s 🟩
permission-check 4s 🟩
permission-check 3s 🟩

settingsfeedbackdocs ⋅ learn more about trunk.io

@rahxephon89
Copy link
Contributor

Hi @movekevin, where do you see these warnings, CI or CLI? I believe this PR already resolves this: #14368 cc @wrwg

Copy link
Contributor

@wrwg wrwg left a comment

Choose a reason for hiding this comment

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

This error is going away with the v2 compiler. Lets better patch the v1 compiler to also omit the warning for self.

@rahxephon89
Copy link
Contributor

This error is going away with the v2 compiler. Lets better patch the v1 compiler to also omit the warning for self.

According to description, this PR seems to be handling the case for V1 compiler @wrwg

Copy link
Contributor

wrwg commented Oct 16, 2024

No, this PR changes the framework source. I suggest changing the compiler.

FWIW I thought we did this already, perhaps just need to use a recent CLI? But if we not we should make this trivial fix to the v1 compiler.

@rahxephon89
Copy link
Contributor

No, this PR changes the framework source. I suggest changing the compiler.

FWIW I thought we did this already, perhaps just need to use a recent CLI? But if we not we should make this trivial fix to the v1 compiler.

Yeah, I mean the same, it should already be fixed by the PR here: #14368. @wrwg

@movekevin
Copy link
Contributor Author

Hi @movekevin, where do you see these warnings, CI or CLI? I believe this PR already resolves this: #14368 cc @wrwg

I saw this in CLI. Updating to the latest version makes the error go away so we're good here. Thanks for flagging the solution!

@movekevin movekevin closed this Oct 16, 2024
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