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 explicit vars and enable --strict-var #541

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

Timmmm
Copy link
Collaborator

@Timmmm Timmmm commented Sep 9, 2024

This adds explicit var keywords where they were missing before, and enables the --strict-var flag to prevent us from missing them out in future. Implicit var was a mistake inherited from ASL.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Sep 9, 2024

Note, a lot of these vars are totally unnecessary and should be lets. In future we should go through the code and audit all vars and undefineds to make sure they are really appropriate.

@Alasdair the Sail compiler could probably benefit from a "var foo is never mutated; use let instead" warning. Or maybe even make it an error.

Copy link

github-actions bot commented Sep 9, 2024

Test Results

712 tests  ±0   712 ✅ ±0   0s ⏱️ ±0s
  6 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 3d1c9ab. ± Comparison against base commit ad8b8cb.

♻️ This comment has been updated with latest results.

This adds explicit `var` keywords where they were missing before, and enables the `--strict-var` flag to prevent us from missing them out in future. Implicit `var` was a mistake inherited from ASL.
Copy link
Collaborator

@Alasdair Alasdair left a comment

Choose a reason for hiding this comment

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

LGTM

@Alasdair
Copy link
Collaborator

I added the warning you suggested, but it looks like you fixed the place where it would occur already!

Warning: Unnecessary mutability model/riscv_fetch.sail:38.16-21:
38 |                PC_hi : xlenbits = PC + 2;
   |                ^---^
   | 
This variable is mutable, but it is never modified. It could be declared as immutable using 'let'.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Sep 10, 2024

Ha, I thought there would be more in the vector code but I guess those are all tuples where one part is modified.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Sep 10, 2024

@billmcspadden-riscv ok if I merge this?

@billmcspadden-riscv billmcspadden-riscv merged commit d905c9c into riscv:master Sep 10, 2024
2 checks passed
@billmcspadden-riscv
Copy link
Collaborator

billmcspadden-riscv commented Sep 10, 2024 via email

@Alasdair
Copy link
Collaborator

I think most of the vector code has the following shape:

var result = undefined;
var mask = undefined;

(result, mask) = init

If it was like var (result, mask) = init it would warn on mask being unmodified I think.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Sep 10, 2024

Ah cool, I'll go through and change them all at some point and then hopefully we'll see the warning.

@Timmmm Timmmm deleted the user/timh/strict_var branch September 23, 2024 09:56
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.

4 participants