Issue 1475: Add support for VHDL 2019 views#1477
Issue 1475: Add support for VHDL 2019 views#1477jeremiah-c-leary merged 3 commits intojeremiah-c-leary:masterfrom
Conversation
linknum23
force-pushed
the
views-2019
branch
3 times, most recently
from
8fcf3ff to
c5340ff
Compare
Resolves jeremiah-c-leary#1475
| """ | ||
| interface_signal_declaration ::= | ||
| [ signal ] identifier_list : [ mode ] subtype_indication [ bus ] [ := *static*_expression ] | ||
| signal identifier_list : mode_indication |
There was a problem hiding this comment.
I replaced [ signal ] with signal since the case where signal is absent is handled by interface_unknown_declaration. Unsure which is more correct. Depending on your stance a comment about the change would probably be useful here.
There was a problem hiding this comment.
I see your point. Line 15 requires the signal keyword, so including the braces in the comment is misleading.
Thanks for clarifying this in the comment.
|
|
||
|
|
||
| def classify(iToken, lObjects): | ||
| def detect(iToken, lObjects): |
There was a problem hiding this comment.
detect was needed for handling the options in element_mode_indication
linknum23
changed the title
jeremiah-c-leary
left a comment
jeremiah-c-leary
left a comment
There was a problem hiding this comment.
Evening @linknum23 ,
You are first person dig in to the parser. I hope it wasn't too difficult to figure out.
The PR looks great. You followed the patterns perfectly and it looks exactly what I would have written.
There are just some dead code and cosmetic updates.
Thanks again.
Regards,
--Jeremy
| def classify(iToken, lObjects): | ||
| """ | ||
| simple_mode_indication ::= | ||
| [ mode ] subtype_indication [ bus ] [ := static_expression ] |
There was a problem hiding this comment.
Change static_expression to static_conditional_expression to match the LRM
There was a problem hiding this comment.
IIRC reason that I changed this was matching other similar LRM comments that left out the italicized portion of the keyword. I think the italicized part of the LRM is added just provide extra context.
I'll change this back to match the LRM.
| """ | ||
| interface_signal_declaration ::= | ||
| [ signal ] identifier_list : [ mode ] subtype_indication [ bus ] [ := *static*_expression ] | ||
| signal identifier_list : mode_indication |
There was a problem hiding this comment.
I see your point. Line 15 requires the signal keyword, so including the braces in the comment is misleading.
Thanks for clarifying this in the comment.
vsg/token/simple_mode_indication.py
Outdated
| super().__init__(sString) | ||
|
|
||
|
|
||
| class expression(parser.expression): |
There was a problem hiding this comment.
I believe this can be removed because the expression production is called. I'm assuming this is a copy/paste from another file before I starting using the expression produciton.
There was a problem hiding this comment.
IIRC the goal here was to be able to use a subtyped expression for rules in the future. I'm trying to remember if this specificity is needed in the codes current state.
vsg/token/simple_mode_indication.py
Outdated
| from vsg import parser | ||
|
|
||
|
|
||
| class subtype_indication(parser.subtype_indication): |
There was a problem hiding this comment.
I believe this can be removed as this token is not used in the production.
|
|
||
| lTokens = [] | ||
| lTokens.append(token.interface_unknown_declaration.assignment) | ||
| lTokens.append(token.simple_mode_indication.assignment) |
There was a problem hiding this comment.
This actually fixes a bug since the original did not include interface_signal_declaration.
vsg/parser.py
Outdated
| super().__init__(sString) | ||
|
|
||
|
|
||
| class mode_indication(item): |
There was a problem hiding this comment.
This can be removed as it does not appear to be used.
|
Thanks for looking at this. The that this follows the LRM made the parser fairly easy to modify. I'm hoping I can find some time to trickle in some more 2019 changes. |
It's all part of my master plan. If it is easy enough for me to figure out, then anybody else can.
Looking forward them. I will get this merged to master. --Jeremy |
2 participants
Resolves #1475
There are probably some additional rules that need to be updated, but my current repo didn't report any issues with these changes.
I realize that this is a fairly complicated change and am open to feedback on how to do it better.