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

Add a comment on a not strictly necessary grammar rule #3016

Conversation

jafingerhut
Copy link
Contributor

Also remove redundant test local_instance.p4, which was identical
except for comments to control-instance.p4

Also remove redundant test local_instance.p4, which was identical
except for comments to control-instance.p4
@jnfoster
Copy link
Contributor

jnfoster commented Jan 4, 2022

I'm not sure the error message is more helpful.

If we accept the proposed language change, and disallow instantiations in blocks, wouldn't it be better to do it in the grammar instead of the type system?

Also, are we certain this change is backward compatible? I'm not...

@jafingerhut
Copy link
Contributor Author

I have no strong opinion on the matter. I created this PR following a suggestion from Mihai, but happy to change it to something else if someone has recommendations.

@mihaibudiu
Copy link
Contributor

I don't think we'll reject any program that used to be accepted.
We'll just reject it in a different way.

@jnfoster
Copy link
Contributor

jnfoster commented Jan 4, 2022

Can you point me to the line in the spec where we say that instantiations within a block are not allowed? (It's probably there, my memory is just faulty.)

@mihaibudiu
Copy link
Contributor

There isn't any, but the implication of the evaluation section is that all instantiations are "static" - performed at compile-time.
So having them in a block statement is not really well-defined, since the block statements have a dynamic scope.
Or course, they could be treated like C statics, which are declared within blocks, and this restricts their visibility but not their lifetime, which is still static.

@jafingerhut
Copy link
Contributor Author

@jnfoster This PR was created in response to this proposed change to the P4 spec that would explicitly prohibit this: p4lang/p4-spec#1006

Adding the comment to the p4c grammar change should probably be made if and only if that PR or something similar is approved.

@jnfoster
Copy link
Contributor

jnfoster commented Feb 7, 2022

It would be nice to signal the error directly in the Bison grammar...

@jafingerhut
Copy link
Contributor Author

I have no problems if someone can find a better change to make to p4c for this. If it involves issuing error messages from the Bison grammar, I think I'm currently ill-prepared to write it. All of the changes in this PR are under Apache 2.0 license as far as I am concerned, so feel free to reuse as much or as little of this PR as it helps someone else to write a different one, and we can close this if it becomes useless.

@jafingerhut
Copy link
Contributor Author

@mihaibudiu @jnfoster p4lang/p4-spec#1006 was merged into P4 language spec in early 2022.

Is it thus reasonable to consider merging this PR?

@mihaibudiu
Copy link
Contributor

I am all for it

@jnfoster jnfoster merged commit cfb3210 into p4lang:main Aug 22, 2023
16 checks passed
@jnfoster
Copy link
Contributor

Agree. I've gone ahead and merged this as it's a non-substantive change.

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