search

p4lang / p4c

P4_16 reference compiler
https://p4.org/
Apache License 2.0
665 stars 440 forks source link

Clean up P4C namespaces #4707

Open fruffy opened 3 months ago

fruffy commented 3 months ago

Our namespace policy is a little unclear. Currently, we seem to use P4 for front/mid end, ControlPlaneAPI for the control plane, and IR for the IR code. The lib folder has no namespaces.

This namespace policy makes it more difficult to link P4C as a library and also introduce elements that can be used across the entire compiler framework without polluting the global namespace.

It might be worthwhile to step back and figure out a clearer policy on namespaces. The difficulty is that any changes to the core code will be a major breaking change.

qobilidop commented 2 months ago

I'm starting to use P4C as a library at work, and this has come up as a major issue.

In the short term, in order to minimize downstream code changes, I wonder if it makes sense to introduce an extra top-level namespace P4C (personally I'd prefer to use p4c, but P4C seems more consistent with existing convention)? Current namespaces will become nested namespaces like P4C::IR. The benefit is that any existing code can do a one-line using namespace P4C; change, and expect the rest of the code to work.

After this refactoring, I guess figuring out a clearer namespace policy can be left as a longer term issue to solve over time.

ChrisDodd commented 2 months ago

Currently, most things in the compiler should be in one of a few namespaces:

fruffy commented 2 months ago

I'm starting to use P4C as a library at work, and this has come up as a major issue.

In the short term, in order to minimize downstream code changes, I wonder if it makes sense to introduce an extra top-level namespace P4C (personally I'd prefer to use p4c, but P4C seems more consistent with existing convention)? Current namespaces will become nested namespaces like P4C::IR. The benefit is that any existing code can do a one-line using namespace P4C; change, and expect the rest of the code to work.

After this refactoring, I guess figuring out a clearer namespace policy can be left as a longer term issue to solve over time.

We could make a PR to get started with this and then see what breaks etc.

qobilidop commented 2 months ago

Sure, I can draft a PR for this.

qobilidop commented 1 month ago

Just want to have some further discussion regarding #4825 and follow-ups.

My goal for #4825 is to serve as a good starting point for further clean-ups. The diffs are intentionally kept as small and straightforward as possible, leaving a lot of room for further improvements.

So after #4825 is merged, maybe we could do one round of more careful review & refactoring on a per-subdirectory basis? If there are a few of us available for this work, I propose that we coordinate in advance who will review which subdirectory, so we can approach this in parallel, and don't double the work by accident.

What does everyone think?

fruffy commented 1 month ago

What does everyone think?

Any concrete cleanups you have in mind? I can do this ad-hoc for P4Tools at the very least.

qobilidop commented 1 month ago

Any concrete cleanups you have in mind? I can do this ad-hoc for P4Tools at the very least.

Some concrete items I can think of now:

In general, I feel that once we start going over each directory more carefully, we might be able to identify more namespace issues or room for further cleanups.

Also, I can sign up to review the following directories that I feel I have a relatively good understanding of:

qobilidop commented 1 month ago

Reopen as this issue is not fully resolved. Keep it to track further namespace cleanups.

fruffy commented 1 month ago

Reopen as this issue is not fully resolved. Keep it to track further namespace cleanups.

Feel free to edit the original post to create an umbrella issue or create an entirely new one.

vlstill commented 3 weeks ago

Please update https://github.com/p4lang/p4c/blob/main/.gdbinit to call dump/dbprint/... from the new namespace.