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.

Sign up for GitHub

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

Jump to bottom

Support of Digest extern in P4TC #4842

Merged
merged 1 commit into from
Aug 7, 2024
Merged

Support of Digest extern in P4TC #4842

merged 1 commit into from
Aug 7, 2024

Conversation

komaljai
Copy link
Contributor

@komaljai komaljai commented Aug 1, 2024

No description provided.

@komaljai komaljai requested a review from Sosutha August 1, 2024 03:21
@komaljai komaljai added the p4tc Topics related to the P4-TC back end label Aug 1, 2024
@komaljai komaljai force-pushed the p4tc_digest_extern branch from 64f40b4 to 6b3b9a9 Compare August 1, 2024 03:50
asl
asl reviewed Aug 1, 2024
View reviewed changes
backends/tc/backend.cpp Outdated Show resolved Hide resolved
backends/tc/backend.cpp Outdated Show resolved Hide resolved
backends/tc/backend.cpp Outdated Show resolved Hide resolved
backends/tc/backend.cpp Outdated Show resolved Hide resolved
backends/tc/backend.cpp Outdated Show resolved Hide resolved
@komaljai komaljai force-pushed the p4tc_digest_extern branch 2 times, most recently from 1f17323 to 2f75b14 Compare August 1, 2024 05:58
@fruffy fruffy added the run-sanitizer Use this tag to run a Clang+Sanitzers CI run. label Aug 1, 2024
asl
asl reviewed Aug 1, 2024
View reviewed changes
backends/tc/backend.cpp Outdated Show resolved Hide resolved
@komaljai komaljai force-pushed the p4tc_digest_extern branch from 2f75b14 to a02145b Compare August 1, 2024 08:54
@komaljai komaljai requested a review from asl August 1, 2024 13:34
asl
asl reviewed Aug 2, 2024
View reviewed changes
backends/tc/tc.def Outdated Show resolved Hide resolved
@asl
Copy link
Contributor

asl commented Aug 2, 2024

String changes are ok to me (see small suggestions).

Sosutha
Sosutha approved these changes Aug 2, 2024
View reviewed changes
Copy link
Contributor

@Sosutha Sosutha left a comment

Choose a reason for hiding this comment

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

Looks fine

@Sosutha Sosutha added this pull request to the merge queue Aug 2, 2024
fruffy
fruffy requested changes Aug 2, 2024
View reviewed changes
Copy link
Collaborator

@fruffy fruffy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please address @asl's recent request before merging

@fruffy fruffy removed this pull request from the merge queue due to a manual request Aug 2, 2024
@komaljai komaljai force-pushed the p4tc_digest_extern branch from 1aee8b6 to 63c837e Compare August 6, 2024 06:25
@fruffy fruffy requested a review from asl August 6, 2024 06:52
@komaljai komaljai force-pushed the p4tc_digest_extern branch from 63c837e to 54aeec6 Compare August 6, 2024 07:01
asl
asl requested changes Aug 6, 2024
View reviewed changes
Copy link
Contributor

@asl asl left a comment

Choose a reason for hiding this comment

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

Looks like the code changes were purely mechanical. The code will benefit a lot, if it will be changed to do string operations in better way

backends/tc/tc.def Outdated Show resolved Hide resolved
backends/tc/tc.def Outdated Show resolved Hide resolved
backends/tc/tc.def Outdated Show resolved Hide resolved
backends/tc/tc.def Outdated Show resolved Hide resolved
backends/tc/tc.def Outdated Show resolved Hide resolved
backends/tc/tc.def Outdated Show resolved Hide resolved
backends/tc/tc.def Outdated Show resolved Hide resolved
backends/tc/tc.def Outdated Show resolved Hide resolved
backends/tc/tc.def Outdated Show resolved Hide resolved
backends/tc/tc.def Outdated Show resolved Hide resolved
asl
asl previously requested changes Aug 6, 2024
View reviewed changes
Copy link
Contributor

@asl asl left a comment

Choose a reason for hiding this comment

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

Looks like you decide to ignore all my comments just resolving all of them. I doubt this is how review process work.

I'm leaving this for @fruffy

backends/tc/tc.def Outdated Show resolved Hide resolved
@asl asl dismissed their stale review August 6, 2024 16:38

Not going to review this anymore as author is not willing to address review comments

@komaljai
Support of Digest extern in P4TC
e65ad5a 
Signed-off-by: Y <komal.jain@intel.com>
@komaljai komaljai force-pushed the p4tc_digest_extern branch from 54aeec6 to e65ad5a Compare August 7, 2024 05:21
@komaljai
Copy link
Contributor Author

komaljai commented Aug 7, 2024 •
edited
Loading

Hi @asl , I did address all comments. As they were many comments, I tried to mark resolved as I fixed each locally. My bad. The unit tests were failing in the end, due to which I couldn't push yesterday. I have pushed them now.

fruffy
fruffy approved these changes Aug 7, 2024
View reviewed changes
Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

Seem there was a misunderstanding here but I believe most complaints are resolved now.

@komaljai
Copy link
Contributor Author

komaljai commented Aug 7, 2024

Seem there was a misunderstanding here but I believe most complaints are resolved now.

Thanks @fruffy

@komaljai komaljai added this pull request to the merge queue Aug 7, 2024
Merged via the queue into p4lang:main with commit c338851 Aug 7, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asl asl asl left review comments

@fruffy fruffy fruffy approved these changes

@Sosutha Sosutha Sosutha approved these changes

Assignees
No one assigned
Labels
p4tc Topics related to the P4-TC back end run-sanitizer Use this tag to run a Clang+Sanitzers CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@komaljai @asl @fruffy @Sosutha