-
Notifications
You must be signed in to change notification settings - Fork 637
Remove deprecated annotation APIs #4717
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
Conversation
1a043e7
to
1a7dbe2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
def annotations: ArrayBuffer[ChiselAnnotation] = dynamicContext.annotations | ||
def annotations: ArrayBuffer[() => Seq[Annotation]] = dynamicContext.annotations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a very basic level, this kind of cleanup is great. Previously, we were using a class that was wrapping a function. Now we are directly using a function. 💯
annotations.foreach(annotate(_)) | ||
// annotate doesn't support InstanceId (which is deprecated) because InstanceId doesn't implement toRelativeTarget | ||
// this API is deprecated anyway so probably fine to not check it. | ||
annotate()(Seq(SinkAnnotation(component.toNamed, name)) ++ Option.when(disableDedup)(NoDedupAnnotation(moduleName))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems okay to me. Also, given that this is deprecated since 6.0, delete in a follow-on? 💣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was exactly my thinking, I just figured it should be a separate PR
* Remove ChiselAnnotation and ChiselMultiAnnotation * Remove annotate methods for ChiselAnnotations * Remove Circuit APIs from ChiselOptions and ChiselCircuitAnnotation * Use ElaboratedCircuit instead * Remove ChiselEnum related annotations
1a7dbe2
to
c5f0b60
Compare
This feature is being removed from Chisel 7: chipsalliance/chisel#4717 These custom annotations aren't used by `firtool`, so their removal doesn't impact that tool's outputs.
Enables #4644
Contributor Checklist
docs/src
?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x
,5.x
, or6.x
depending on impact, API modification or big change:7.0
)?Enable auto-merge (squash)
and clean up the commit message.Create a merge commit
.