Skip to content

[#1362] Clarify that switch statements _are_ allowed in action/function bodies, and that switch statements with action_run expressions are only allowed in control apply blocks #1363

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

Merged
merged 3 commits into from
Mar 11, 2025

Conversation

kfcripps
Copy link
Contributor

@kfcripps kfcripps commented Feb 4, 2025

Cleans up some wording that was missed by #945, which makes it unclear as to whether switch statements are actually supported in actions/functions or not.

Closes #1362.

@vlstill
Copy link
Contributor

vlstill commented Feb 4, 2025

So this does work in the P4C, right?

@vlstill vlstill self-requested a review February 4, 2025 19:12
@kfcripps
Copy link
Contributor Author

kfcripps commented Feb 4, 2025

So this does work in the P4C, right?

@vlstill For backends that don't run the EliminateSwitch midend pass, yes, it already works. The p4c frontend passes produce valid IR from a switch statement.

Copy link
Contributor

@vlstill vlstill left a 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, thanks for the clarifications.

This type of `switch` statement can only be used within control `apply`
blocks. For this variant of `switch` statement, the expression must be
of the form `t.apply().action_run`, where `t` is the name of a table
(see <<sec-invoke-mau>>). All switch labels must be names of
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this implicit from the fact that table.apply() can only be used in control apply blocks, not in functions or actions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

"This type of switch statement can only be used within control apply blocks." is definitely implied by other parts of the spec, but it seems reasonable to say here, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also don't think it hurts to explicitly state that here, but if anyone strongly opposes it I can remove it.

The other clarifications are the most important parts of this PR.

Copy link
Collaborator

@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

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

LGTM

@kfcripps
Copy link
Contributor Author

kfcripps commented Mar 3, 2025

I can't merge, so feel free to merge if everyone is happy with the changes.

@jafingerhut
Copy link
Collaborator

@jonathan-dilorenzo Discussed at 2025-Mar-10 and everyone that approves still approves, and ChrisDodd is good with the little bit of redundancy introduced as well. Please take a look, and merge if you also approve.

As far as we know, p4c already supports this, although there are ongoing discussions on achieving good test coverage for different back ends that prefer different transformations for switch statements before the backend is reached. Those discussions only affect p4c test coverage, not the language of this spec change.

@jonathan-dilorenzo
Copy link
Collaborator

@kfcripps, can I ask you to add a bullet point summarizing the change to this section? https://p4.org/p4-spec/docs/p4-16-working-draft.html#_summary_of_changes_made_in_unreleased_version

Happy to merge after that!

@kfcripps
Copy link
Contributor Author

@jonathan-dilorenzo I realized that I did not add a bullet to this section in #1358, so I also added a bullet summarizing the changes made by #1358.

@@ -9046,6 +9048,9 @@ The P4 compiler should provide:

=== Summary of changes made in unreleased version

* Clarified that numeric priorities cannot be assigned to entries of a table that has `const entries` (<<sec-entries>>).
* Clarify that `switch` statements are allowed in action and function bodies, and that `switch` statements with `action_run` expressions are only allowed in control `apply` blocks (<<sec-stmts>> and <<sec-switch-stmt>>).
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: clarified*

Let's stick to the same tense ;). O.w. LGTM

@jonathan-dilorenzo
Copy link
Collaborator

Much obliged on both accounts! Thanks Kyle!

Signed-off-by: Kyle Cripps <[email protected]>
Copy link
Collaborator

@jonathan-dilorenzo jonathan-dilorenzo left a comment

Choose a reason for hiding this comment

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

Looks great!

@jonathan-dilorenzo jonathan-dilorenzo merged commit 75aca49 into p4lang:main Mar 11, 2025
1 check passed
@kfcripps kfcripps deleted the 1362 branch March 11, 2025 19:29
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.

Clarify whether P4-16 allows switch statements in actions
5 participants