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

Improve source locators for switch statements. #1669

Merged
merged 3 commits into from
Nov 16, 2020
Merged

Improve source locators for switch statements. #1669

merged 3 commits into from
Nov 16, 2020

Conversation

danielkasza
Copy link
Contributor

This fixes #1648.

Contributor Checklist

  • N/A 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?
  • N/A 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?
  • N/A Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • bug fix

API Impact

No impact.

Backend Code Generation Impact

The source locators in the code generated from the switch statements will no longer reference Conditional.scala. They will now reference the source line of the is statement.

Desired Merge Strategy

  • Squash

Release Notes

N/A

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (3.2.x, 3.3.x, 3.4.0, 3.5.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

@danielkasza danielkasza requested a review from a team as a code owner November 13, 2020 02:59
@danielkasza danielkasza requested review from jackkoenig and removed request for a team November 13, 2020 02:59
@CLAassistant
Copy link

CLAassistant commented Nov 13, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

Awesome!

This will definitely help traceability when using switch. Nice first patch!

src/main/scala/chisel3/util/Conditional.scala Outdated Show resolved Hide resolved
src/test/scala/chiselTests/SwitchSpec.scala Outdated Show resolved Hide resolved
src/test/scala/chiselTests/SwitchSpec.scala Outdated Show resolved Hide resolved
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

Looks great!

@seldridge seldridge modified the milestone: 3.2.x Nov 13, 2020
@seldridge
Copy link
Member

@jackkoenig: Can this be backported and how far? This is adding a second parameter group which I think will break backwards compatibility?

@seldridge seldridge added this to the 3.5.0 milestone Nov 16, 2020
@seldridge seldridge merged commit 87916d5 into chipsalliance:master Nov 16, 2020
jackkoenig added a commit that referenced this pull request Feb 28, 2023
* Build ArrayBuffers in Block.mapStmt

* Have empty Block serialize as "skip"

The FIRRTL parser requires at least one indented line in each module.
Sometimes tests emit and parse modules with no contents; this ensures
there's always at least a "skip" in empty modules.

Also fix tests that expected certain skips

* Use var List as stack in Block.mapStmt impl

This replaces Iterator concatenation. In Scala 2.11, RHS recursion on
Iterators is not stack safe. This seems to have been fixed in 2.12 by
Scala PR 5033.

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch statements should not point to "Conditional.scala" in generated verilog
3 participants