-
Notifications
You must be signed in to change notification settings - Fork 80
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
Allow concatenation of string literals at compile time #1299
base: main
Are you sure you want to change the base?
Conversation
Resulting PDF spec P4-16-spec.pdf. |
Signed-off-by: Vladimir Still <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
f2e412f
to
27fef74
Compare
@jafingerhut, @jonathan-dilorenzo, @rcgoodfellow, any comments? I'd love for this one to get in before the next spec revision goes out. |
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.
Mostly LGTM. Just a few nits.
p4-16/spec/P4-16-spec.mdk
Outdated
deprecated construct. This is mostly useful for annotating library | ||
constructs, such as externs. | ||
deprecated construct. This is mostly useful for annotating library | ||
constructs, such as externs. The parameter needs to be local |
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: needs to be local -> needs to be a local
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.
Would also go with 'must be' over 'needs to be' imo.
p4-16/spec/P4-16-spec.mdk
Outdated
@@ -8602,7 +8627,8 @@ extern Checker { | |||
The `noWarn` annotation has a required string argument that indicates | |||
a compiler warning that will be inhibited. For example | |||
`@noWarn("unused")` on a declaration will prevent a compiler warning | |||
if that declaration is not used. | |||
if that declaration is not used. The parameter needs to be local |
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: needs to be local -> needs to be a local
p4-16/spec/P4-16-spec.mdk
Outdated
The type `string` represents strings. There are no operations on | ||
string values; one cannot declare variables with a `string` type. | ||
Parameters with type `string` can be only directionless (see Section | ||
The type `string` represents strings. The values are either string |
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.
It's a bit strange to think of concatenations as being values... Maybe "string-typed expressions are either string literals or concatenations of multiple string-typed expressions."?
p4-16/spec/P4-16-spec.mdk
Outdated
@@ -1304,8 +1304,8 @@ number of backslash characters (ASCII code 92). P4 does not make any | |||
validity checks on strings (i.e., it does not check that strings | |||
represent legal UTF-8 encodings). | |||
|
|||
Since P4 does not provide any operations on strings, | |||
string literals are generally passed unchanged through the P4 compiler to | |||
Since P4 does not allow strings to exist at runtime, string literals |
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.
I think this isn't really true anymore? Now we also concatenate them e.g.? And I'm guessing we plan to turn that into a single string literal before passing it along to backends (though maybe not?).
I think the point of this paragraph is to say that the P4 compiler won't interpret string literals. What does that mean in the new world with concatenation?
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.
Probably terminating quotes are no longer left as-is?
p4-16/spec/P4-16-spec.mdk
Outdated
|
||
The only operation allowed on strings is concatenation, denoted by | ||
`++`. For string concatenation, both operands must be strings and | ||
the result is also a string. String concatenation can be only |
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: can only be* performed
p4-16/spec/P4-16-spec.mdk
Outdated
body. In the | ||
following example, the fully-qualified name of the table is `c_inst.t1`. | ||
language element from the control plane. This annotation takes a local | ||
compile-time know value of type `string` (typically a string literal). |
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: know -> known*
p4-16/spec/P4-16-spec.mdk
Outdated
deprecated construct. This is mostly useful for annotating library | ||
constructs, such as externs. | ||
deprecated construct. This is mostly useful for annotating library | ||
constructs, such as externs. The parameter needs to be local |
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.
Would also go with 'must be' over 'needs to be' imo.
Signed-off-by: Vladimir Still <[email protected]>
Thanks for the comments and sorry for the delayed response. I think I've fixed all the issues now. |
@vlstill Sorry for the inconvenience, but we have, after several months of planning, transitioned the P4 language specification source from Madoko to AsciiDoc. Thus most or all of your PRs will need to be updated so that they apply to that version. Hopefully this should be fairly straightforward, e.g. by looking at examples of how things like section headings, bullet items, etc. are marked up in the latest version of the file P4-16-spec.adoc If you have questions on how to change things for AsciiDoc, feel free to ask. Note that creating a new PR with similar changes as this PR might be easier than updating this PR. |
@jafingerhut, no problem, I've refreshed the PR for the new asciidoc documentation. |
@jonathan-dilorenzo, @rcgoodfellow this was already discussed, now there is just updated wording in the new template, can we get this merged now? |
This gives wording to #1297, the compile-time concatenation of strings. The P4C part of this is p4lang/p4c#4856. The changes are relatively small, I hope I have not missed any more parts that mention that there are no string operations. Note that as stated here, it also allows string concatenation in
@name
,@deprecated
and@noWarn
pragmas. The last one has probably no use for it, but I think it is better to be consistent there.Resolves #1297.