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

rename-internal #514

Merged
merged 2 commits into from
Nov 1, 2024
Merged

rename-internal #514

merged 2 commits into from
Nov 1, 2024

Conversation

naure
Copy link
Collaborator

@naure naure commented Oct 31, 2024

The program column for immediates has become complex and it cannot be summarized with a name, so rename it to "internal".

Copy link
Collaborator

@matthiasgoergens matthiasgoergens left a comment

Choose a reason for hiding this comment

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

I appreciate the sentiment, but I think the right conclusion to draw here is that the field doesn't make sense and should be replaced by something saner.

If you are interested, I can give you a detailed proposal and even a PR.

@naure
Copy link
Collaborator Author

naure commented Oct 31, 2024

If you are interested, I can give you a detailed proposal and even a PR.

If you have something simpler in mind, let's hear it. But changing this any further is really low priority. Especially given the low test coverage and low type safety.

Tests would be welcome!
More type safety too!

@matthiasgoergens
Copy link
Collaborator

matthiasgoergens commented Oct 31, 2024

My proposal would fix some of the safety issues.

But it sounds like you wouldn't review a proposal (and no one else here would either). So I don't think I care about pushing yet another languishing PR into the void at this point in time.

Your proposal here doesn't make the existing code worse as far as I can tell. So I don't mind it.

@naure
Copy link
Collaborator Author

naure commented Nov 1, 2024

No if you have it in mind already do write it as an issue.

Then, the first rule of refactoring is to avoid regressions by increasing tests and documentation coverage. That has great benefits and no drawbacks which is why I'm suggesting that direction now.

Copy link
Collaborator

@hero78119 hero78119 left a comment

Choose a reason for hiding this comment

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

Nice! the new naming hide much information and looks for simpler now 👍

@KimiWu123
Copy link
Contributor

imm_internal is more confusing for me. 1. what _internal means?, 2 'imm_or_funct7does returnsfunct7` in R-type.

@naure
Copy link
Collaborator Author

naure commented Nov 1, 2024

It can contain funct7 but also others things depending on instructions. So it's a generic container with a generic name.

@naure
Copy link
Collaborator Author

naure commented Nov 1, 2024

Mixed opinions on this, but I do want people to go learn the "internal" logic, instead of assuming something from an imprecise name.

@naure naure enabled auto-merge (squash) November 1, 2024 08:06
@naure naure merged commit 8ad25c5 into master Nov 1, 2024
6 checks passed
@naure naure deleted the rename-internal branch November 1, 2024 08:08
@matthiasgoergens
Copy link
Collaborator

No if you have it in mind already do write it as an issue.

See #524

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