chisel icon indicating copy to clipboard operation
chisel copied to clipboard

Make some usages of suggestName runtime deprecated

Open mwachs5 opened this issue 3 years ago • 4 comments

Contributor Checklist

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

Type of Improvement

  • code cleanup

API Impact

This runtime deprecates some usages of the suggestName API and warns that they will be turned into errors in the future. Specifically, you can no longer:

  • call suggestName on the same HasId more than once
  • call suggestName after containing Module close
  • call suggestName with the same value that the prefix (autoSeed) would have given it
  • call suggestName outside of a builder context
  • Call suggestName on something that is not actually nameable (such as a subfield of an aggregate)

Backend Code Generation Impact

No impact on output verilog, though people avoiding these runtimeDeprecations would see them.

Desired Merge Strategy

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

Release Notes

Some usages of suggestName are runtime deprecated or made an error

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.

mwachs5 avatar Jun 02 '22 16:06 mwachs5

Opening this as draft since I am still working on the tests, which are revealing to me that there are myriad usages of suggestName inside the chisel internals itself that I was not aware of

mwachs5 avatar Jun 02 '22 16:06 mwachs5

Note this currently comments out reflective naming as it kept tripping up the "you are suggesting the same name the plugin would have given you" test

mwachs5 avatar Jun 03 '22 09:06 mwachs5

This has changed in that I now am checking that thing-you-are-suggestNaming is actually canBeNamed (brought this back from #2561 ), and no longer checking "has name already been used before suggestname", because i'm not sure the suggestion for that given that we don't want to waste space on _computedName... probably simpler to just check if we have already closed the module

mwachs5 avatar Jun 19 '22 23:06 mwachs5

I think I've incorporated all the feedback

mwachs5 avatar Jul 02 '22 03:07 mwachs5