Skip to content
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

Revise general explanation on type casting #1354

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jaehyun1ee
Copy link
Contributor

Following the discussion made at #1351, this PR revises the general explanation on type casting given in section 8.11. Casts* to the following:

;; Before

P4 provides a limited set of casts between types. A cast is written
`(t) e`, where `t` is a type and `e` is an expression. Casts are only
permitted on base types and derived types introduced by `typedef`, `type`, and `enum`.
While this design is arguably more onerous for programmers, it has several benefits:
;; After

P4 provides a limited set of casts between types. A cast is written
`(t) e`, where `t` is a type and `e` is an expression. Casts are only permitted in
cases as defined in section <<sec-explicit-casts>> for explicit casts and
section <<sec-implicit-casts>> for implicit casts. While this design is arguably
more onerous for programmers, it has several benefits:

Rather than stating "Casts are only permitted on base types and derived types introduced by typedef, type, and enum." (which is quite inaccurate), it puts forward references to the following subsections. Because:

  • Not all base types can be casted (e.g., void, error, match_kind, and string)
  • Derived types, such as struct and header can be casted, e.g., at initialization.

@vlstill vlstill self-requested a review January 13, 2025 21:55
@jafingerhut
Copy link
Collaborator

At 2025-Jan-13 vlstill agreed to review in detail. No other LDWG members could think of reasons to object based on our discussion, but they should review and add their comments if they wish.

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.

I agree with the new wording. For the sake of readability of this area of the spec I suggest some tweaks to the following text.

p4-16/spec/P4-16-spec.adoc Outdated Show resolved Hide resolved
Comment on lines 3847 to 3848
section <<sec-implicit-casts>> for implicit casts. While this design is arguably
more onerous for programmers, it has several benefits:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the last sentence makes little less sense with the forward references. I suggest something like this:

Suggested change
section <<sec-implicit-casts>> for implicit casts. While this design is arguably
more onerous for programmers, it has several benefits:
section <<sec-implicit-casts>> for implicit casts. Main goals of this design are:

and possibly add

  • It limits implicit casts to the bare minimum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion, I revised the sentences as:

Casts in P4 are permitted only under the conditions specified in section sec-explicit-casts for explicit casts and section sec-implicit-casts for implicit casts. By allowing casts only in limited scenarios, this restriction is more onerous for programmers, but it also provides several benefits.

I tried to make it explicit that casts are very limited in P4.

p4-16/spec/P4-16-spec.adoc Outdated Show resolved Hide resolved
Signed-off-by: Jaehyun Lee <[email protected]>
Comment on lines +3848 to +3849
By allowing casts only in limited scenarios, this restriction is more
onerous for programmers, but it also provides several benefits.
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: maybe this would read more naturally:

Suggested change
By allowing casts only in limited scenarios, this restriction is more
onerous for programmers, but it also provides several benefits.
Allowing casts only in limited scenarios is more onerous for
programmers, but it also provides several benefits.

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.

3 participants