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

Deprecate ChiselStage$.elaborate #3160

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

seldridge
Copy link
Member

Deprecate ChiselStage$.elaborate as this exposes internal Chisel APIs, namely "chisel3.internal.firrtl.Circuit". Replace usages of this in tests with emitCHIRRTL.

Comment on lines 214 to 217
@deprecated(
"this exposes the internal Chisel circuit which was not supposed to be public---use either ChiselStage.convert or ChiselStage.emitCHIRRTL instead ",
"Chisel 5.0"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@deprecated(
"this exposes the internal Chisel circuit which was not supposed to be public---use either ChiselStage.convert or ChiselStage.emitCHIRRTL instead ",
"Chisel 5.0"
)
@deprecated(
"this exposes the internal Chisel circuit which was not supposed to be public--use either ChiselStage.convert or ChiselStage.emitCHIRRTL instead",
"Chisel 5.0"
)

Minor nit on the message, feel free to ignore

Copy link
Member Author

Choose a reason for hiding this comment

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

The use of a fixed-width em-dash was intentional as opposed to a fixed-width en-dash which would not be appropriate. I could use alternative punctuation, though. The space was unintentional. I'll remove that.

@mwachs5
Copy link
Contributor

mwachs5 commented Apr 4, 2023

seems like we have lots of tests where we don't actually care about the emission, and just want to see if we can successfully elaborate. Could we make something like testElaborate or checkElaborate just return Unit or true/false? Does emitChirrtl always emit a file, which could be annoying...

@seldridge
Copy link
Member Author

ChiselStage$.emitCHIRRTL or ChiselStage$.convert do not write any files. I considered adding a test-only elaborate method to do this, but it seemed unnecessary given that there was an existing method which we could re-use here.

Deprecate ChiselStage$.elaborate as this exposes internal Chisel APIs,
namely "chisel3.internal.firrtl.Circuit".  Replace usages of this in
tests with emitCHIRRTL.

Signed-off-by: Schuyler Eldridge <[email protected]>
@seldridge seldridge force-pushed the dev/seldridge/deprecate-ChiselStage.elaborate branch from ca38905 to 1f75a06 Compare April 4, 2023 23:54
@seldridge seldridge enabled auto-merge (squash) April 4, 2023 23:54
@seldridge seldridge merged commit 3a59415 into main Apr 5, 2023
@seldridge seldridge deleted the dev/seldridge/deprecate-ChiselStage.elaborate branch April 5, 2023 00:16
@jackkoenig jackkoenig added the Deprecation Deprecates an API, will be included in release notes label Apr 6, 2023
jared-barocsi pushed a commit that referenced this pull request Apr 11, 2023
Deprecate ChiselStage$.elaborate as this exposes internal Chisel APIs,
namely "chisel3.internal.firrtl.Circuit".  Replace usages of this in
tests with emitCHIRRTL.

Signed-off-by: Schuyler Eldridge <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecation Deprecates an API, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants