-
Notifications
You must be signed in to change notification settings - Fork 82
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 cast for directionless args #1341
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jaehyun Lee <[email protected]>
From LDWG: This is clarifying and making something more explicit that was implicit in the spec before. We have tests currently that have this assumption baked in, and |
inserts implicit casts for direction `in` or directionless arguments to methods or | ||
functions as described in <<sec-casts>>. The types for all |
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.
One more thing, the sentence says "to methods or functions", but there are two more cases that could be relevant:
- directionless arguments of actions with argument explicitly supplied in P4 (I think you have mentioned that for this case the casts are already allowed somewhere);
- arguments of extern and control/parser instances are are directionless arguments, and implicit casts could be applied here too (i.e.
Register<bit<32>, bit<3>>(8, 42)
-- the intializer type is presumablybit<32>
, but one expectsint
argument will work).
So essentially I would suggest this should be allowed in any cases where a directionless argument can happen (except for directionless argument value coming from control plane, but there I can't see a way how one could want to cast it).
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.
Thank you for the comment :) For arguments to control/parser instances, I understand them as arguments to the "apply" method of the control/parser instances. So I think it roughly corresponds to the explanation "... or directionless arguments to method or functions as described ...". For example, the spec mentions:
To invoke the services of another control, it must be first instantiated; the services of an instance are invoked by calling it using its apply method. (14.4)
It would be nice to enumerate all different kinds of invocations somewhere in the spec, perhaps in the section describing the calling convention. Yet, I don't have a good idea regarding, exactly where in that section, and with what explanation should we insert.
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.
If anything, invocations such as Register<bit<32>, bit<3>>(8, 42)
are invocations of constructors, which apparently spec does consider to be methods.
7.2.10.2. Extern objects:
Extern object declarations can also optionally declare constructor methods;
But for action, for some reason it is its own thing, different from a method or function (arguably it is a bit different, because action is implicitly a closure which has access to all elements of the control it is defined in, while function is completely freestanding and method has only access to the extern object itself (if anything)). So I think at least actions should be mentioned here (i.e. "methods, actions or functions").
I would like to propose allowing implicit cast for directionless method / function / constructor arguments, following the discussion made in #1312.
Regarding it, the current spec section 8.20 mentions that:
So as per the spec, we cannot apply implicit cast for directionless method / function / constructor arguments.
As a side note, directionless action arguments can be implicitly cast as per the spec since it mentions the following in section 6.8.1.
So, like the case for action, I am proposing to allow implicit cast for directionless method / function / constructor arguments.
I believe it better reflects the use cases in the p4c test suite. Below is a drop-down list of tests that expect such a feature.
List of tests