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

[p4fmt]: Add a pretty printer for p4fmt. #4862

Merged
merged 17 commits into from
Aug 22, 2024
Merged

[p4fmt]: Add a pretty printer for p4fmt. #4862

merged 17 commits into from
Aug 22, 2024

Conversation

snapdgn
Copy link
Contributor

@snapdgn snapdgn commented Aug 10, 2024

pretty printer for p4fmt

fruffy
fruffy reviewed Aug 10, 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.

I would call it a different name. P4Formatter? Also feel free to prune as many members and methods as possible. I think most of them are not needed here.

Also add a preorder function for const IR::Node * throw a P4C_UNIMPLEMENTED exception there.

@snapdgn
Copy link
Contributor Author

snapdgn commented Aug 10, 2024 •
edited
Loading

I would call it a different name. P4Formatter? Also feel free to prune as many members and methods as possible. I think most of them are not needed here.

Sure.

Also add a preorder function for const IR::Node * throw a P4C_UNIMPLEMENTED exception there.

Yes, was planning to do this.

@snapdgn
Copy link
Contributor Author

snapdgn commented Aug 13, 2024

I would call it a different name. P4Formatter?

Just to clarify things, did you mean I should rename 'toP4' class to 'P4Formatter' ?

@fruffy
Copy link
Collaborator

fruffy commented Aug 13, 2024

I would call it a different name. P4Formatter?

Just to clarify things, did you mean I should rename 'toP4' class to 'P4Formatter' ?

Yes, or whatever name you find most appropriate.

@fruffy
Copy link
Collaborator

fruffy commented Aug 15, 2024

Need to update the namespace after #4825 has been merged, then we are good to go.

@fruffy
Copy link
Collaborator

fruffy commented Aug 16, 2024 •
edited
Loading

@snapdgn is this ready to be merged? It is still in draft mode.

@snapdgn
[p4fmt]: pretty printer for p4fmt
2524925 
Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn
remove nested namespace from 'p4fmt'
46e1275 
Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn
rename 'toP4' to 'P4Formatter'
55abd59 
Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn
update header guard
50f8683 
Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn
add a catchall preoder method
d496c8e 
Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn
clean member variables & methods
7e9280f 
Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn
mark single args constructors as explicit
dded809 
Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn
rebase & update namespace
7800b5e 
Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn snapdgn marked this pull request as ready for review August 16, 2024 08:15
@snapdgn
Copy link
Contributor Author

snapdgn commented Aug 16, 2024

@snapdgn is this ready to be merged? It is still in draft mode.

Sorry, this should be ready now.

qobilidop
qobilidop approved these changes Aug 16, 2024
View reviewed changes
Copy link
Member

@qobilidop qobilidop left a comment

Choose a reason for hiding this comment

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

LGTM other than the directory structure.

backends/p4fmt/p4formatter/p4formatter.h Outdated Show resolved Hide resolved
@snapdgn
restructure p4fmt dir
ecbd6b7 
Signed-off-by: Nitish <snapdgnn@proton.me>
asl
asl previously requested changes Aug 16, 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.

See comments

backends/p4fmt/p4formatter/p4formatter.cpp Outdated Show resolved Hide resolved
backends/p4fmt/p4formatter/p4formatter.cpp Outdated Show resolved Hide resolved
backends/p4fmt/p4formatter/p4formatter.cpp Outdated Show resolved Hide resolved
backends/p4fmt/p4formatter.cpp Outdated Show resolved Hide resolved
backends/p4fmt/p4formatter.cpp Outdated Show resolved Hide resolved
backends/p4fmt/p4formatter.cpp Outdated Show resolved Hide resolved
backends/p4fmt/p4formatter.cpp Outdated Show resolved Hide resolved
backends/p4fmt/p4formatter.cpp Outdated Show resolved Hide resolved
backends/p4fmt/p4formatter.cpp Outdated Show resolved Hide resolved
backends/p4fmt/p4formatter.cpp Outdated Show resolved Hide resolved
@snapdgn
Address PR review suggestions [partial]
c74bbdb 
Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn
Refactor repetitive collection iteration with a reusable `visitCollec…
e9c2c9e 
…tion` function

Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn
remove top level qualified P4 namespace
4b22b49 
Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn
[visitCollection]: deal with the enumerator type
197bdd8 
Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn
use RAII objects for separator management
e4e1db8 
Signed-off-by: Nitish <snapdgnn@proton.me>
@snapdgn
use std::filesystem::path for path manipulations
76e121d 
Signed-off-by: Nitish <snapdgnn@proton.me>
fruffy
fruffy approved these changes Aug 20, 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.

@asl I will go ahead and merge this today if you do not have any other comments.

backends/p4fmt/p4formatter.h Outdated Show resolved Hide resolved
backends/p4fmt/p4formatter.h Outdated Show resolved Hide resolved
backends/p4fmt/p4formatter.h Outdated Show resolved Hide resolved
@fruffy fruffy changed the title [p4fmt]: pretty printer for p4fmt [p4fmt]: Add a pretty printer for p4fmt. Aug 20, 2024
snapdgn and others added 2 commits August 20, 2024 13:23
@snapdgn
nits
cbfa732 
Signed-off-by: Nitish <snapdgnn@proton.me>
@fruffy
Merge branch 'main' into pp-p4fmt
cf63b9f
@fruffy fruffy enabled auto-merge August 20, 2024 17:10
@fruffy
Copy link
Collaborator

fruffy commented Aug 22, 2024

Dismissing @asl's review to merge, we can address remaining issues in subsequent PRs.

@fruffy fruffy dismissed asl’s stale review August 22, 2024 07:03

Most comments have been addressed.

@fruffy fruffy added this pull request to the merge queue Aug 22, 2024
@snapdgn
Copy link
Contributor Author

snapdgn commented Aug 22, 2024

Most comments have been addressed.

Yes, I don't think I missed resolving any of the suggestions.

Merged via the queue into p4lang:main with commit bba10d7 Aug 22, 2024
18 checks passed
@snapdgn snapdgn deleted the pp-p4fmt branch August 22, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fruffy fruffy fruffy approved these changes

@qobilidop qobilidop qobilidop approved these changes

@asl asl Awaiting requested review from asl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@snapdgn @fruffy @asl @qobilidop