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 annotations on functions #4452

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Conversation

ChrisDodd
Copy link
Contributor

Probably an oversight that this wasn't allowed before. Use case is to specify which hardware resource(s) should be used to implement a specific function

@jafingerhut
Copy link
Contributor

Good addition. Can you add a simple test program using this feature, too?

@jafingerhut
Copy link
Contributor

Do you want to create an issue and/or PR for the language spec, too? If you would prefer somone else do that, I can do it.

@ChrisDodd
Copy link
Contributor Author

Do you want to create an issue and/or PR for the language spec, too? If you would prefer somone else do that, I can do it.

The spec is a bit unclear, but seems to allow annotations on any 'element' in a program, but it is not clear what is and is not an element. Besides this case, p4c currently does not allow annotations on error and match_kind, nor on any statement except for block statements (allowing them on statements that start with a ( would be problematic, parsing wise).

@jafingerhut
Copy link
Contributor

Do you want to create an issue and/or PR for the language spec, too? If you would prefer somone else do that, I can do it.

The spec is a bit unclear, but seems to allow annotations on any 'element' in a program, but it is not clear what is and is not an element. Besides this case, p4c currently does not allow annotations on error and match_kind, nor on any statement except for block statements (allowing them on statements that start with a ( would be problematic, parsing wise).

Agreed that the spec English prose can be a bit unclear on this point. The part that would be worth updating in the spec is the grammar.mdk file that is currently almost the same as the Bison grammar used by p4c. This PR updates the grammar used by p4c, so making corresponding changes in the grammar.mdk file of the spec would be good.

@ChrisDodd
Copy link
Contributor Author

Good addition. Can you add a simple test program using this feature, too?

I can add a testcase defining a trivial function with a random annotation, but that really only tests that the perser accepts it (which is pretty trivial -- not sure if it is worth the extra 1 second of CI time)

@fruffy fruffy added the p4-spec Topics related to the P4 specification (https://github.com/p4lang/p4-spec/). label Feb 25, 2024
@ChrisDodd
Copy link
Contributor Author

I added a spec (grammar) change to match #1274

Copy link
Contributor

@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

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

LGTM, although I am not the best person to review the C++ parts of the changes, which is most of this :-)

@ChrisDodd ChrisDodd added this pull request to the merge queue Mar 13, 2024
Merged via the queue into p4lang:main with commit b256c2a Mar 13, 2024
17 checks passed
@ChrisDodd ChrisDodd deleted the cdodd-func-annot branch March 13, 2024 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants