Skip to content

visitor draft - #4786

Closed
ktiefe wants to merge 2 commits into
shogun-toolbox:developfrom
ktiefe:feature/refactor_SGObject_hacks_solution
Closed

visitor draft#4786
ktiefe wants to merge 2 commits into
shogun-toolbox:developfrom
ktiefe:feature/refactor_SGObject_hacks_solution

Conversation

@ktiefe

@ktiefe ktiefe commented Oct 25, 2019

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread src/shogun/base/SGObject.h Outdated

Any::register_visitor<InterfaceType, InterfaceVisitor>([] (auto value, auto visitor) {
visitor->value = value;
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the idea is to put this in watch_param? @vigsterkr

@vigsterkr vigsterkr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to think about how to deal with arrays

{
auto* result = sgo_details::get_by_tag(this, name, sgo_details::GetByNameIndex(index));
//auto* result = sgo_details::get_by_tag(this, name, sgo_details::GetByNameIndex(index));
auto* result = sgo_details::get_by_tag(this, name);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem here that imo this ignores the index parameter. so the visitor that you've written only works in the expected manner for CSGObject::get(std::string_view name, std::nothrow_t). i need to see how we support get CSGObject* CSGObject::get(std::string_view name, index_t index) const. that's basically when the registered parameter is an array

{
return get_dispatch_all_base_types(obj, name, how);
}
CSGObject* get_by_tag(const CSGObject* obj, std::string_view name) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this whole thing could be deleted and go into a private function that is implemented in .cpp

BaseTag tag(name);
create_parameter(tag, AnyParameter(make_any_ref(value), properties));

if constexpr (is_sg_base<T>::value) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo if you wrap this whole block into a constexpr function then you could just call that instead of copy pasting things 4 times.

#endif // DOXYGEN_SHOULD_SKIP_THIS
#endif // SWIG

struct InterfaceVisitor

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might be better to place into a separate .h file and include it here. place it under src/shogun/util/visitors

@vigsterkr

Copy link
Copy Markdown
Member

closing in favour of #4793
sorry @ktiefe needed this fix...

@vigsterkr vigsterkr closed this Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants