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

Allow implicit conversion of brace-lists to type list #4599

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ChrisDodd
Copy link
Contributor

An explicit list type has been in the spec for awhile, but p4c was requiring explicit casts on {}-lists to match them to list typed thing in type unification.

This small change allow unification of {} lists with type list without an explicit cast.

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 don't think this adheres to the current language spec. If I look correctly, this would essentially be an implicit cast from a tuple expression to a list expression (P4 lists should be syntactically written as (list<T>){...} while {...} is a tuple). The spec does not define such casts in https://p4.org/p4-spec/docs/P4-16-v1.2.4.html#sec-casts.

I was tempted to suggest adding this to the spec, but on closer thought I don't think it would be a great addition. If a heterogeneous tuple was passed to a function that expects a list, the typing errors could be quite weird. Also implicit casts in general tend to create a lot of problems in languages that use them liberally (like C++) so I am quite glad P4 is quite restrictive in their use.

There are probably not enough brace characters to give different syntax to tuples, lists, structs, ... One improvement could be to allow syntax like list{a, b, c} (or (list){a, b, c}) as a shorthand for (list<T>){a, b, c} where T is the type of a.

Comment on lines +495 to +496
if (elTypes.count(t)) continue;
elTypes.insert(t);
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid double-search:

Suggested change
if (elTypes.count(t)) continue;
elTypes.insert(t);
if (!elTypes.insert(t).second) continue;

@fruffy fruffy added core Topics concerning the core segments of the compiler (frontend, midend, parser) p4-spec Topics related to the P4 specification (https://github.com/p4lang/p4-spec/). labels Apr 5, 2024
@ChrisDodd
Copy link
Contributor Author

ChrisDodd commented Apr 8, 2024

The spec doesn't really say what a brace enclosed list by itself is, so allowing it to be implicitly converted to a tuple or a struct or a header type, but not implicitly converted to a list seems inconsistent.

Most programmers coming from a C background will expect to be able to use brace-enclosed lists to initialize both array-like things and a struct-like things. Its also somewhat painful to need an explicit cast here when it is clear from context what is going on.

@vlstill
Copy link
Contributor

vlstill commented May 9, 2024

The spec doesn't really say what a brace enclosed list by itself is, so allowing it to be implicitly converted to a tuple or a struct or a header type, but not implicitly converted to a list seems inconsistent.

Could be, but in that case we should update the spec first.

Most programmers coming from a C background will expect to be able to use brace-enclosed lists to initialize both array-like things and a struct-like things. Its also somewhat painful to need an explicit cast here when it is clear from context what is going on.

C is not exactly known for a nice type system. But you have a point here, many languages have some kind of initializer syntax which can be many different things based on context. But the rules should be precisely defined by the spec -- are there cases where a tuple-like initializer can be passed to an argument with parametric types? Are there cases where we would need to give priority or reject the cast because there are multiple variants? The latter probably not since we don't have type-based overloading if I look correctly.

@fruffy
Copy link
Collaborator

fruffy commented May 15, 2024

Should we create a spec issues here?

@ChrisDodd ChrisDodd mentioned this pull request May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser) p4-spec Topics related to the P4 specification (https://github.com/p4lang/p4-spec/).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants