-
Notifications
You must be signed in to change notification settings - Fork 87
Add arrays of non-header types #1360
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
Conversation
|
Should we in this PR also define literal syntax for arrays, e.g. perhaps something like |
|
In this section: there is an explicit statement about accessing elements of header stacks that are outside of range: Should that be generalized to reading an element of an array where the index is out of range for the array? Similarly later in that section for attempted writes to a header stack element it says: Should that also be generalized to attempting to write to elements of an array where the index is out of bounds? |
vlstill
left a comment
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.
Looks good to me, I agree we should have exact semantics of out-of-bounds access as suggested in #1360 (comment).
|
This opens up the possibility of having nested arrays, which could potentially complicate array / header stack processing in some passes. We might consider explicitly prohibiting nested arrays until there is real motivation to support them. Related discussion: p4lang/p4c#5115 (comment) |
Signed-off-by: Chris Dodd <[email protected]>
|
Most of the support for this is already in p4c -- last remaining piece is p4lang/p4c#5338 |
|
Ready for review and merging. @jonathan-dilorenzo and @jafingerhut to review. |
|
Let's resolve the "error?" to error and make a note that this isn't done for any deep reason, but simply due to the lack of compelling use cases and the ability to change our minds in a non-breaking way. |
|
|
||
| A header stack represents an array of headers or header unions. A header stack type is | ||
| defined as an array declaration | ||
| where `typeRef` is the name of a header or header union type. For a |
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.
Considering typedefs which may alias header or header union types, e.g.,
header Hdr { bit<32> x }
typedef Hdr hhh;
... and somewhere else we use hhh[5], a header stack"where typeRef is the name of a header or header union type" is a bit imprecise.
So maybe we can put:
A header stack type is defined as an array declaration where `typeRef` represents
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.
+1. I think that phrasing is quite nice.
a8ce860 to
5b80553
Compare
jonathan-dilorenzo
left a comment
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.
Minor comments, but approving and will merge immediately upon fix.
| | `list types` | error | error | error | allowed | error [7] | error | ||
|
|
||
| | `list types` | error | error | error | allowed | error | ||
| | `extern instances` | error | error | error | erroe | allowed [6] | error |
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.
nit: error*
|
|
||
| A header stack represents an array of headers or header unions. A header stack type is | ||
| defined as an array declaration | ||
| where `typeRef` is the name of a header or header union type. For a |
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.
+1. I think that phrasing is quite nice.
Comment that these combos do not appear useful, but don't seem hazardous either, so could be added in the future. Signed-off-by: Chris Dodd <[email protected]>
No description provided.