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

Enforce more consistent type naming #197

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Enforce more consistent type naming #197

wants to merge 7 commits into from

Conversation

micprog
Copy link
Member

@micprog micprog commented Jan 19, 2022

Unify type naming across modules, using axi_req_t for AXI request and axi_lite_req_t for axi lite request. May fix some issues for vivado (see HERO). Similar for resp types.

@micprog micprog requested a review from andreaskurth January 19, 2022 15:34
Copy link
Contributor

@andreaskurth andreaskurth left a comment

Choose a reason for hiding this comment

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

I see the need for this change: especially the resp_t type collides with types of the same name in many tools. (This is a bug in the respective tool; if the tool implemented type resolution correctly according to the System Verilog standard, there would be no collision. However, as we cannot expect all tool vendors to fix this bug in the foreseeable future, we have to do our best to not trigger it.)

However, I have two concerns:

  1. The types should be named according to the scheme we discussed in Harmonize ports and parameters #153.
  2. This change will break many usages of our modules. We therefore need to be careful about its rollout. We probably want to do one release where we make all the breaking changes agreed upon in Harmonize ports and parameters #153, so that users only have to adapt their code once. However, this will require more work.

What do you think?

@micprog
Copy link
Member Author

micprog commented Jan 20, 2022

I was unaware of #153, but aligning to this makes sense, thanks for pointing me in that direction. I agree that limiting the breaking changes to one release is the "cleanest" approach. I can convert this branch to a draft, and we could have this as a working branch for the changes, if that makes sense?

@andreaskurth
Copy link
Contributor

andreaskurth commented Jan 20, 2022

I was unaware of #153, but aligning to this makes sense, thanks for pointing me in that direction. I agree that limiting the breaking changes to one release is the "cleanest" approach. I can convert this branch to a draft, and we could have this as a working branch for the changes, if that makes sense?

Yes please!

@micprog micprog marked this pull request as draft January 20, 2022 17:04
@thommythomaso thommythomaso self-requested a review August 30, 2022 13:45
@micprog micprog force-pushed the type_naming branch 5 times, most recently from eaf3e9e to 8e172ec Compare April 10, 2023 19:28
micprog added 4 commits April 10, 2023 21:44
Unify type naming across modules, using `axi_req_t` for AXI request and
`axi_lite_req_t` for axi lite request. May fix some issues for vivado
(see HERO). Similar for `resp` types.
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