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

STIL - Allow waveform defs with no semicolon #203

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ginty
Copy link
Member

@ginty ginty commented Jun 26, 2024

In AMD we have some historical STIL files which do not terminate waveform events with a ; in cases where there was only a single event.

Until recently the OM STIL parser handled that OK (unintentionally), but recent updates made by @rlaj have made it fail to parse such definitions (again unintentionally).

This update relaxes the allowed syntax vs. the spec slightly by allowing the final event to have no semicolon at the end.

Added parsing test cases for this and the tagged waveform definitions which were previously un-tested.

Copy link
Member

@priyavadan priyavadan left a comment

Choose a reason for hiding this comment

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

we discussed this , I have my thoughts but I understand the need/sentiment and am okay with the changes.

Copy link
Member

@coreyeng coreyeng left a comment

Choose a reason for hiding this comment

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

Since it is outside the spec, would it be prudent to add an option for this (I don't mind which is the default)? Something like either use_strict_stil_spec = true/false, or relax_stil_spec = true/false and setup the parser accordingly?

Copy link
Member

@rlaj rlaj left a comment

Choose a reason for hiding this comment

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

Approving, but have no problem with @coreyeng proposal. Although, I think there are some other gaps the parser has w.r.t. to the spec, so use_strict_stil_spec=true might give a false sense of security.

@ginty
Copy link
Member Author

ginty commented Jun 26, 2024

Since it is outside the spec, would it be prudent to add an option for this (I don't mind which is the default)? Something like either use_strict_stil_spec = true/false, or relax_stil_spec = true/false and setup the parser accordingly?

Yeah that's fair, let me see if I can come up with a way to do this

@coreyeng
Copy link
Member

@rlaj, I'm not that familiar with STIL and you're probably right. I put that more in "we don't know what we don't know" category. If we do have an option though, we can lump other things we aren't thinking about in there. A "strict STIL parser" may be a WiP but we'll have that option to cover things we use that are outside of that.

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.

4 participants