Closed mmaloney-sf closed 2 months ago
Technically this is only mandatory as of 4.0.0, https://github.com/chipsalliance/firrtl-spec/pull/191 . However I think just requiring this regardless is fine re:compatibility.
I too am interested in impact of this.
cc https://github.com/chipsalliance/firrtl-spec/issues/188 .
@dtzSiFive Thanks for pointing that out. I feel like I removed that verbiage and then forgot it.
And thank you for pointing out that I didn't feature-gate this. I'm not sure how I should proceed on that point. I had plans to add a lot of small tweaks to the syntax, but it could easily make it a bit of spaghetti to keep strict backwards compat. But then again, I don't know much people care about that compatibility.
Note: no version of Chisel that I have ever seen has ever not emitted commas unconditionally. There should be no risk in changing this and it should be backwards compatible.
Please add explicit tests that dangling commas in lists and before optional components are rejected.
Done.
If there are no other comments, could someone merge?
Sounds good to me, @uenoku
Due to historical circumstances, the SFC parser for FIRRTL treated commas (
,
) as whitespace. This behavior was carried over into the CIRCT implementation offirtool
.This behavior is utterly bizarre.
Moreover, the FIRRTL spec has indicated comma tokens (
,
) for at least every version since last year.This PR removes this quirk and properly tokenizes commas, requiring them in the places dictated by the FIRRTL spec.
The impact of this PR should be low, but it may break code (in CIRCT, in Chisel, and elsewhere) in places where FIRRTL was being emitted with a loose interpretation, or where the spec was ambiguous.
Changes:
','
from the list of "horizontal whitespace" and added an explicitFIRToken::comma
token.parse*()
methods to consume the tokens.first
to skip parsing comma tokens on the first iteration. Please check me here, since I learned C++ before lambdas were a thing.smem
).layer
decls need commas. I opted for commas. I can swap if I got this backwards.parseRUW()
as a non-optional variant ofparseOptionalRUW()
, since the comma token can be used to determine optionality in one case.