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

Remove & rename duplicate word_width <-> bytes mappings #436

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

Timmmm
Copy link
Collaborator

@Timmmm Timmmm commented Mar 19, 2024

  • Remove the duplicate mapping functions.
  • Rename to size_bytes for consistency with the other two functions.
  • Rename size_bits to size_enc, otherwise it's quite confusing that size_bits is not just 8 * size_bytes.

Fixes #423

Copy link

github-actions bot commented Mar 19, 2024

Test Results

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

Results for commit 2c82c30. ± Comparison against base commit a2ffa85.

♻️ This comment has been updated with latest results.

* Remove the duplicate mapping functions.
* Rename to `size_bytes` for consistency with the other two functions.
* Rename `size_bits` to `size_enc`, otherwise it's quite confusing that `size_bits` is not just `8 * size_bytes`.

Fixes riscv#423
@Timmmm Timmmm force-pushed the user/timh/word_width_bytes branch from fec3215 to 2c82c30 Compare April 4, 2024 11:40
@Timmmm
Copy link
Collaborator Author

Timmmm commented Apr 9, 2024

@billmcspadden-riscv can we get this landed too? It's a very simple refactoring - no functional changes - but I have a number of upcoming PRs that depend on this.

@billmcspadden-riscv
Copy link
Collaborator

@Timmmm: A couple of comments:

  1. While I agree with you that the function name, size_bits(), is not the best, I'm not sure that size_enc() is much better, although it begins to tell you what it's being used for (an encoding of some type.) Can we change the name to something that says it's the word width encoding in bits?
  2. Since we're changing names of a generally used function, I'd like to hear from other contributors about the nomenclature change. @arichardson , do you have an opinion of this change?

@arichardson
Copy link
Collaborator

I agree that size_enc is a lot better than size_bits (which I would expect to return 8/16/etc.). I agree with @billmcspadden-riscv that it's still a somewhat confusing name, but I can't think of anything better so I'm happy with this change. Maybe @jrtc27 or @Alasdair have other suggestions.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Apr 11, 2024

Arguably word_width is the confusing name here since RISC-V universally uses "word" to mean "32-bits" not any of the other possible meanings.

But anyway since this is an pareto improvement (i.e. it makes some things better and nothing worse) can we land it and then potentially use better names in a future PR?

@billmcspadden-riscv
Copy link
Collaborator

billmcspadden-riscv commented Apr 12, 2024 via email

@billmcspadden-riscv billmcspadden-riscv merged commit e187e02 into riscv:master Apr 12, 2024
2 checks passed
@Timmmm Timmmm deleted the user/timh/word_width_bytes branch May 15, 2024 08:42
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.

Duplicate mappings bytes_wordwidth and word_width_bytes
3 participants