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 1 commit 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.

Signed-off-by: Jaehyun Lee <99jaehyunlee@gmail.com>
@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.

Comment on lines +3845 to +3846
`(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
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this sounds a little better?

Suggested change
`(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
`(t) e`, where `t` is a type and `e` is an expression. Casts are only permitted in
cases defined in section <<sec-explicit-casts>> for explicit casts and

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.

Comment on lines 3854 to 3855
* It reduces the number of cases that have to be considered in the P4
specification. Some targets may not support all casts.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say the last one should be separate. and probably not a bulletpoint.

Suggested change
* It reduces the number of cases that have to be considered in the P4
specification. Some targets may not support all casts.
* It reduces the number of cases that have to be considered in the P4
specification.
Note: some targets may not support all casts.

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