Allow extending ToP4, clean its constructors #4899
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vlstill
added
the
breaking-change
vlstill
force-pushed
the
vstill/top4-extensions
branch
from
82ccc21 to
81ce8f4
Compare
We'd probably go with some value-based semantics here. But it seems pretty heavyweight for a task. And essentially the same wrapped smart pointer. :) |
fruffy
reviewed
Sep 4, 2024
frontends/common/parser_options.cpp
Outdated
| @@ -472,8 +472,8 @@ static std::filesystem::path makeFileName(const std::filesystem::path &folder, | |||
|
|
|||
| bool ParserOptions::isv1() const { return langVersion == ParserOptions::FrontendVersion::P4_14; } | |||
|
|
|||
| void ParserOptions::dumpPass(const char *manager, unsigned seq, const char *pass, | |||
| const IR::Node *node) const { | |||
| void ParserOptions::dumpPass(ToP4Factory toP4Fact, const char *manager, unsigned seq, | |||
There was a problem hiding this comment.
Make this a virtual function of the options that returns the appropriate ToP4 instance? E.g., ToP4 * toP4Printer(). Then you can also change the priner with the options you are using.
There was a problem hiding this comment.
Good idea. I left the unique_ptr there as I think we should not rely on GC especially in the places where the memory management is very straight-forward
fruffy
added
the
core
qobilidop
approved these changes
Sep 5, 2024
Signed-off-by: Vladimir Still <[email protected]>
Signed-off-by: Vladimir Still <[email protected]>
Signed-off-by: Vladimir Still <[email protected]>
Signed-off-by: Vladimir Still <[email protected]>
vlstill
force-pushed
the
vstill/top4-extensions
branch
from
81ce8f4 to
41e4bf0
Compare
Signed-off-by: Vladimir Still <[email protected]>
vlstill
force-pushed
the
vstill/top4-extensions
branch
from
41e4bf0 to
3c78563
Compare
fruffy
approved these changes
Sep 22, 2024
ChrisDodd
approved these changes
Sep 23, 2024
vlstill
commented
Sep 23, 2024
Signed-off-by: Vladimír Štill <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cstringargument inToP4withstd::filesystem::path. This is the breaking change.ParserOptionswith a descendant ofToP4-- the way I did this is not exactly nice, but I don't see other clear way to create the instance on demand.ToP4pass is a good candidate for larger refactoring, but I don't have time for that now.