-
Notifications
You must be signed in to change notification settings - Fork 23
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
Verilog attribute parsing OpenSTA #11
Verilog attribute parsing OpenSTA #11
Conversation
Signed-off-by: Ethan Mahintorabi <ethanmoon@google.com>
Signed-off-by: Ethan Mahintorabi <ethanmoon@google.com>
Signed-off-by: Ethan Mahintorabi <ethanmoon@google.com>
Signed-off-by: Ethan Mahintorabi <ethanmoon@google.com>
Signed-off-by: Ethan Mahintorabi <ethanmoon@google.com>
Signed-off-by: Ethan Mahintorabi <ethanmoon@google.com>
Signed-off-by: Ethan Mahintorabi <ethanmoon@google.com>
b5b542d
to
2a379d1
Compare
Signed-off-by: Ethan Mahintorabi <ethanmoon@google.com>
Thanks for the feedback. Addressed all comments. After moving to std::string I needed to wrap it in std::optional in order to preserve an unset value check. I the new API overall, but if you don't we can go back to const char * or return an empty std::string when the attribute is not found. |
Signed-off-by: Ethan Mahintorabi <ethanmoon@google.com>
I am having a hard time seeing a reason to distinguish between an empty string and a missing value. I don't think the std::optional is very useful here. Please use a "using" to get rid of all the std::'s. I think string is already covered by StringUtil.hh |
I think the question is, is this a valid attribute I think I tend to agree with you though. It seems like in most other place we don't differentiate between empty string and not present. I'll go back to just |
This reverts commit 8aef4d5. Signed-off-by: Ethan Mahintorabi <ethanmoon@google.com>
Signed-off-by: Ethan Mahintorabi <ethanmoon@google.com>
All feedback has been addressed. |
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.
ship it
Cool, Merge whenever you're ready. I don't have any permission on this repo |
Hey Ethan @QuantamHD , I am finding that this PR results in a parsing error when >2 attributes are present on a line:
To reproduce, try a multi-attribute statement like: |
Allows STA to read, parse and write verilog attributes, example below.