Use normal composition in flavor classes #788
Comments
|
why you not backed by an array now but future is ok ? |
|
I'm sorry, I don't understand the question. The array backing was originally done as a way to get iteration but we have since moved to another way with listing the fields in a macro. In the future we'd want these to be as normal classes as possible. |
codygunton
pushed a commit
to AztecProtocol/aztec-packages
that referenced
this issue
Nov 30, 2023
Reduces duplication in Flavor classes by using a macro that combines pointer_view with struct fields (along with a new get_all getter that is perhaps more ergonomic) and subclassing. Note that this is a stepping stone, ideally AztecProtocol/barretenberg#788 is used instead of multiple inheritance Also resolves AztecProtocol/barretenberg#784 - Introduces a RefVector class, like `std::vector<T&>` (which is impossible). It is backed by a pointer, and gives operator[] and iterator semantics on the underlying derefenced pointer. Compare std::vector<std::reference_wrapper<T>>, which would operate more like actual pointers (you change the reference, not the value held by the reference). - Introduces a get_all alternative to pointer_view that uses RefVector, update other types to use RefVector, get rid of the HandleType in flavor entity classes - Removes some flavor base classes that didn't do much after array backing removal. Now we get rid of virtual base class methods here, but I don't think it makes sense to do polymorphism in two ways. We can instead use concepts if we want to constrain this with a static assert - Introduces macros that define flavor classes in flavor_macros.hpp. These remove duplication of pointer view and normal listing. - Use inheritance to remove duplication in the larger AllEntities classes in GoblinUltra and GoblinTranslator - Removes modernize-use-nodiscard - too noisy a linter setting, I doubt we'd hit a bug that it would fix (i.e. if you return error codes it can be useful to not ignore them). --------- Co-authored-by: ludamad <[email protected]>
AztecBot
pushed a commit
that referenced
this issue
Dec 2, 2023
Reduces duplication in Flavor classes by using a macro that combines pointer_view with struct fields (along with a new get_all getter that is perhaps more ergonomic) and subclassing. Note that this is a stepping stone, ideally #788 is used instead of multiple inheritance Also resolves #784 - Introduces a RefVector class, like `std::vector<T&>` (which is impossible). It is backed by a pointer, and gives operator[] and iterator semantics on the underlying derefenced pointer. Compare std::vector<std::reference_wrapper<T>>, which would operate more like actual pointers (you change the reference, not the value held by the reference). - Introduces a get_all alternative to pointer_view that uses RefVector, update other types to use RefVector, get rid of the HandleType in flavor entity classes - Removes some flavor base classes that didn't do much after array backing removal. Now we get rid of virtual base class methods here, but I don't think it makes sense to do polymorphism in two ways. We can instead use concepts if we want to constrain this with a static assert - Introduces macros that define flavor classes in flavor_macros.hpp. These remove duplication of pointer view and normal listing. - Use inheritance to remove duplication in the larger AllEntities classes in GoblinUltra and GoblinTranslator - Removes modernize-use-nodiscard - too noisy a linter setting, I doubt we'd hit a bug that it would fix (i.e. if you return error codes it can be useful to not ignore them). --------- Co-authored-by: ludamad <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Now that we aren't backed by an array, normal composition is possible
The text was updated successfully, but these errors were encountered: