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

Update comments in issue2201-bmv2.p4 to match current behavior #3637

Conversation

jafingerhut
Copy link
Contributor

This makes the example program much more useful to show to people who want to know how BMv2 simple_switch implements the log_msg extern function, as of 2022-Oct (it has not changed in quite some time before that).

Remove issue2201-1-bmv2.p4 since it had strictly less test coverage than issue2201-bmv2.p4.

This makes the example program much more useful to show to people who
want to know how BMv2 simple_switch implements the log_msg extern
function, as of 2022-Oct (it has not changed in quite some time before
that).

Remove issue2201-1-bmv2.p4 since it had strictly less test coverage
than issue2201-bmv2.p4.
Copy link
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

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

no one will find this file with this name

@apinski-cavium
Copy link

no one will find this file with this name

Funny you named testcase that way: #2462 .

I do think we should have some coding style on how test cases are named. naming them after the issue # is not always useful as obvious from this issue and such.

@jafingerhut
Copy link
Contributor Author

jafingerhut commented Oct 29, 2022

"no one will find this file with this name" Agreed. If we want to document the behaviors here at some point in time, e.g. because we want to add the log_msg extern to the PSA and/or PNA specs, then at least this test case will contain accurate information to start that from, as opposed to obsolete information. There is a link to these modified files from such an issue related to the PNA spec, so people working on that spec can find it. Once it is documented elsewhere, they won't need to find this file any more.

@jafingerhut jafingerhut merged commit a0e519e into p4lang:main Oct 29, 2022
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