-
Notifications
You must be signed in to change notification settings - Fork 177
Conversation
@@ -10,6 +10,8 @@ object Serializer { | |||
val NewLine = '\n' | |||
val Indent = " " | |||
|
|||
val version = "1.1.0" |
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.
This would probably be better described as a product and if this was defined globally so this can be used in parser/Listener.scala
:
case class Version(major: Int, minor: Int, patch: Int) extends scala.math.Ordered[Version] {
override def serialize: String = ???
override def compare(that: Version): Bool = ???
}
This would simplify some things later, e.g., exiting if the input version is too new.
Also, should this be defined in IR.scala?
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.
I'm okay with it being in IR.scala
but I'd like to avoid modifying anything in IR.scala if we can avoid it (to avoid backwards compatibility concerns with existing transforms).
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.
I've made a Version
class in Serializer.scala
that is similar to this. Instead of having a compare
method it has an incompatible
method, since I think compare should check the patch version, but we want to be able to accept larger versions as long as only the patch is larger. Let me know if this needs more changes.
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.
I think this is reasonable. If we need to do more work here later we can but this looks good enough to me.
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!
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.
* Parse version and hardcode emitted version * Throw error if version is too high * Parse version even if rest is invalid * Change pattern match to if statement * Improve version grammar * Update tests * Remove outdated comment * Simplify grammar and use version class * Simplify and add no version test * Fix for conflicting lexer rule (cherry picked from commit 19fe90b)
* FIRRTL version support (#2543) * Parse version and hardcode emitted version * Throw error if version is too high * Parse version even if rest is invalid * Change pattern match to if statement * Improve version grammar * Update tests * Remove outdated comment * Simplify grammar and use version class * Simplify and add no version test * Fix for conflicting lexer rule (cherry picked from commit 19fe90b) * Waive binary compatibility breakages and bump previous artifact Co-authored-by: Zachary Yedidia <[email protected]> Co-authored-by: Jack Koenig <[email protected]>
Now that FIRRTL supports version specifiers (chipsalliance/firrtl-spec#30), the parser here should accept versions and the serializer should emit a version. The current version of FIRRTL is 1.1.0. This FIRRTL compiler will throw an error if used to compile FIRRTL with a version greater than 1.1.x (e.g., >= 1.2.0). The serializer currently emits FIRRTL version 1.1.0.
Contributor Checklist
Type of Improvement
API Impact
The serializer will now emit a FIRRTL version number, and the parser will accept one and throw an error if it is above 1.1.0.
Backend Code Generation Impact
No impact.
Desired Merge Strategy
Squash.
Release Notes
The serializer will now emit a FIRRTL version number, and the parser will accept one and throw an error if it is above 1.1.0.
Reviewer Checklist (only modified by reviewer)
Please Merge
?