Refactor the catalog facade API to allow usage outside gaiac #761
Conversation
| { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| std::vector<link_facade_t> table_facade_t::incoming_links() const |
There was a problem hiding this comment.
In an initial version of this change, these methods were templated. This allows specifying different subclasses:
template <class T_link_type = link_facade_t>
std::vector<T_link_type> incoming_links() const
{
auto links = std::vector<T_link_type>();
for (const gaia_relationship_t& relationship : m_table.incoming_relationships())
{
links.emplace_back(relationship, false);
}
return links;
}
So that you can write:
for (auto& link : m_table.outgoing_links<gaiac_outgoing_link_facade_t>())
{
...
}
Where gaiac_outgoing_link_facade_t is a subclass of link_facade_t.
There was a problem hiding this comment.
The reason why I did not go with this approach is that you need to expose the implementation of each method... Because you know.. templates :) If anyone likes this approach we can re-evaluate.
There was a problem hiding this comment.
I would avoid templates unless they are absolutely required.
There was a problem hiding this comment.
Yep, that's what I thought.
| #include "gaia_internal/catalog/gaia_catalog.h" | ||
|
|
||
| /* | ||
| * Contains classes that make it simpler to generate code from the | ||
| * catalog types. Each Catalog class has its own "facade". | ||
| * Eg. gaia_table_t -> table_facade_t. | ||
| * | ||
| * The only user of this class is edc_generator. | ||
| * Each generator (eg. gaiat, gaiac) may decide to subclass one of | ||
| * these classes to provide more specific logics. |
There was a problem hiding this comment.
I assume (and like) that the subclasses are meant to add new methods and not override existing functionality. No need to add virtual functions if we don't expect to use it as a polymorphic type.
There was a problem hiding this comment.
nit: logics -> logic.
Done.
I assume (and like) that the subclasses are meant to add new methods and not override existing functionality. No need to add virtual functions if we don't expect to use it as a polymorphic type.
That's correct.
| std::string generate_incoming_relationships_accessors_cpp(); | ||
| std::string generate_outgoing_relationships_accessors(); | ||
| std::string generate_outgoing_relationships_accessors_cpp(); | ||
| std::string generate_incoming_links_accessors(); |
There was a problem hiding this comment.
So when do we say "link" and when do we say "relationship"?
| { | ||
| auto links = std::vector<link_facade_t>(); | ||
|
|
||
| for (const gaia_relationship_t& relationship : m_table.incoming_relationships()) |
There was a problem hiding this comment.
Should gaia_relationship_t become gaia_link_t?
There was a problem hiding this comment.
gaia_relationship_t should remain as-is. The difference is that we should add gaia_link_t to the relationship.
Eachgaia_relationship_t has 2 gaia_link_t (the forward link and the backward link).
| } | ||
| else | ||
| { | ||
| ASSERT_UNREACHABLE("Unsupported relationship cardinality!"); |
There was a problem hiding this comment.
relationship->link in assert message?
There was a problem hiding this comment.
WIll try to address in next PR.
|
|
||
| std::string gaiac_incoming_link_facade_t::next_offset() const | ||
| { | ||
|
|
There was a problem hiding this comment.
WIll try to address in next PR.
The catalog facade API provides simplified access to the catalog and so far has been used only by
gaiac. This PR (#756) introduced a similar concept ingaiat.@daxhaw pointed out that we could refactor this into a public (internal), and subclass the facade to address specific use cases. I agreed with this approach but to make the #756 lighter, I decided to create a separate PR.
Comments on PR-756:
Note: This API is by no means perfect or final. The idea is that logic to generate code should be placed here (eg. class names, namespaces, etc...). Suggestions, improvements etc.. are all welcome.