-
Notifications
You must be signed in to change notification settings - Fork 444
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
Move ProgramStructure out the BMv2 folder such that it can be used in other back ends. #4770
Conversation
957706b
to
0bde3bf
Compare
98dfbfd
to
128f0e6
Compare
… other back ends. Signed-off-by: fruffy <[email protected]>
128f0e6
to
accdd8e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine
@@ -58,6 +58,17 @@ class V1ModelProperties { | |||
static const cstring validField; | |||
}; | |||
|
|||
// V1Model-specific blocks. | |||
enum class BlockConverted { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the fact that BlockConverted is v1model-specific part of what you consider the "leakiness of the abstraction" mentioned in your comment on this PR?
I am not familiar with the overall structure of the code here, but agree that lifting out common stuff seems like a good idea. I guess one question is how users of this code that are not implementing the v1model architecture replace this definition of BlockConverted
with something else. Perhaps that is something you are hoping is addressed with later PRs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the fact that BlockConverted is v1model-specific part of what you consider the "leakiness of the abstraction" mentioned in your comment on this PR?
Yeah, there are few other parts too. Like the meter map that needs to be part of the top level. Currently, the ProgramStructure still has elements that are v1model-specific. Rewriting code to make ProgramStructure fully generic is a little bit involved, so I am not addressing it in this PR.
The ProgramStructure class in the BMv2 back end was used all across other back ends. The abstraction here was a bit leaky. I pulled it out and moved it in the "common" folder in
backends
. Refactoring suggestions or ideas are welcome here. There are still a bunch of leaky abstractions. The program structure class should ideally work for all back ends.@qobilidop @rupesh-chiluka-marvell