Add indent parameter to Serializer.serialize() #2177
Conversation
Using Utils.indent() gives deprecation warnings to use Serializer instead. However, the Serializer class itself doesn't provide a means to manually indent a FirrtlNode string a certain number of times. The indent variable, previously hardcoded to 0, is now exposed as a second parameter for the modified serialize function, and the old serialize function just calls the modified serialize with indents = 0 for binary compatibility.
|
|
| } | ||
|
|
||
| /** Converts a `FirrtlNode` into its string representation. */ | ||
| def serialize(node: FirrtlNode, indents: Int): String = { |
There was a problem hiding this comment.
| def serialize(node: FirrtlNode, indents: Int): String = { | |
| def serialize(node: FirrtlNode, indents: Int = 0): String = { |
Can't we do away with the above version by just adding a default param? Does that still match binary compatibility? (I don't actually know the answer this is a weak suggestion)
There was a problem hiding this comment.
The default parameter approach was my first attempt at solving this, but I'm not sure how it would be compiled into bytecode. My intuition tells me that the serialize with the default parameter will still be a two-parameter function (with some fancy magic to "insert" the 0 as an argument). So if we swap the firrtl binary then anything else that tries to run that will get a complaint that the one-parameter serialize doesn't exist.
There was a problem hiding this comment.
looks like your intution is right: https://github.com/jatcwang/binary-compatibility-guide#dont-adding-parameters-with-default-values-to-methods
Not sure what the firrtl philosophy is on making this breaking change on master and then fixing in the backport version?
There was a problem hiding this comment.
I think just having the overloaded 2 methods is fine.
I'm going to be super bikesheddy here though and suggest naming the parameter just indent:
| def serialize(node: FirrtlNode, indents: Int): String = { | |
| def serialize(node: FirrtlNode, indent: Int): String = { |
Doing a bit of search on other projects, singular tense is more common for this kind of parameter.
| /** Converts a `FirrtlNode` into its string representation | ||
| * with *zero* indents (old behavior for binary compatibility) */ |
There was a problem hiding this comment.
| /** Converts a `FirrtlNode` into its string representation | |
| * with *zero* indents (old behavior for binary compatibility) */ | |
| /** Converts a `FirrtlNode` into its string representation with default indentation | |
| */ |
A couple of things here:
- Use ScalaDoc style (stars align with the rightmost star from the start of the comment
- Slight tweak to phrasing, the behavior here didn't change so no reason to bring attention to it. There also is indentation if you pass something with things scoped inside it (like a Module or a When), so slightly more precise phrasing.
Put closing comment on new line Co-authored-by: Megan Wachs <megan@sifive.com>
Using Utils.indent() gives deprecation warnings to use Serializer instead. However, the Serializer class itself doesn't provide a means to manually indent a FirrtlNode string a certain number of times. The indent variable, previously hardcoded to 0, is now exposed as a second parameter for the modified serialize function, and the old serialize function just calls the modified serialize with indents = 0 for binary compatibility Co-authored-by: Megan Wachs <megan@sifive.com> (cherry picked from commit 20890bb)
mergify
bot
added
the
Backported
|
Congratulations on your first firrtl PR @jared-barocsi 🎉 |
Add indent parameter to Serializer.serialize() (backport #2177)
Type of Improvement
Code refactoring
API Impact
No impact
Backend Code Generation Impact
Should not impact code generation
Desired Merge Strategy
Squash
Release Notes
Add indentation parameter to
Serializer.serialize()to indentFirrtlNodestrings by a certain amount.Reviewer Checklist (only modified by reviewer)
Please Merge?