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.

Sign up for GitHub

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

Jump to bottom

Enforce port name uniqueness #2567

Merged
merged 9 commits into from
Aug 17, 2022
Merged

Enforce port name uniqueness #2567

merged 9 commits into from
Aug 17, 2022

Conversation

adkian-sifive
Copy link
Contributor

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • code refactoring
  • code cleanup

API Impact

Using suggestName with the same name for multiple ports will now throw an error.

Backend Code Generation Impact

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference.

Release Notes

Port name duplication and port name collision with other components like wires now throws an error instead of silently sanitizing the names.

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (Bug fix: 3.4.x, [small] API extension: 3.5.x, API modification or big change: 3.6.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

jackkoenig
jackkoenig requested changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

forceName has always been convoluted but this makes it a bit worse, I've made some suggestions that may or may not be completely clear so lets chat if you want some clarification.

core/src/main/scala/chisel3/RawModule.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/RawModule.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/Builder.scala Show resolved Hide resolved
core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/RawModule.scala Outdated Show resolved Hide resolved
adkian-sifive and others added 4 commits July 21, 2022 16:31
@adkian-sifive
Update forceName signature
bfebe9a 
Replace namePorts with checkPorts
@adkian-sifive
Negative filter for candidate_name
2b27452
@adkian-sifive
Scalafmt
3fb07f3
@adkian-sifive @jackkoenig
Apply suggestions from code review
ea028f9 
Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>
@adkian-sifive adkian-sifive force-pushed the unique-port-names branch 2 times, most recently from ab0d499 to 1d39d26 Compare July 21, 2022 23:36
jackkoenig
jackkoenig previously requested changes Jul 28, 2022
View reviewed changes
Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

A couple of minor requests but otherwise this is good to go!

core/src/main/scala/chisel3/RawModule.scala Outdated Show resolved Hide resolved
src/test/scala/chiselTests/experimental/ProgrammaticPortsSpec.scala Outdated Show resolved Hide resolved
@adkian-sifive
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jul 28, 2022

update

✅ Branch has been successfully updated

@azidar azidar enabled auto-merge (squash) August 17, 2022 22:13
@azidar azidar merged commit f6db1a6 into chipsalliance:master Aug 17, 2022
@sequencer
Copy link
Member

This PR breaks playground regression CI in https://github.com/chipsalliance/playground/runs/7890002567, which should be fixed in the RC.

@sequencer sequencer mentioned this pull request Aug 22, 2022
Merged
sequencer added a commit to chipsalliance/playground that referenced this pull request Aug 22, 2022
@sequencer
add patch for chipsalliance/chisel#2567
2b9cb45
yuchangyuan pushed a commit to yuchangyuan/playground that referenced this pull request Dec 15, 2022
@sequencer @yuchangyuan
add patch for chipsalliance/chisel#2567
a32878a
jackkoenig pushed a commit that referenced this pull request Feb 28, 2023
@scala-steward @mergify
Update json4s-native to 4.0.6 (#2567)
a72029b 
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mwachs5 mwachs5 mwachs5 left review comments

@azidar azidar azidar approved these changes

@jackkoenig jackkoenig jackkoenig left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adkian-sifive @sequencer @azidar @jackkoenig @mwachs5