Skip to content
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

Full support for std::vectors in Any [WIP] #4392

Closed

Conversation

lisitsyn
Copy link
Member

No description provided.

@lisitsyn lisitsyn requested a review from karlnapf August 18, 2018 12:03
@lisitsyn
Copy link
Member Author

Ok I introduced a test that shows that vectors generally work. What does not work is combinatorial stuff like vectors of SGVectors. Will work on that next

{
std::string str("some string");
std::vector<std::string> lhs{str};
std::vector<std::string> rhs{str};
Copy link
Member

Choose a reason for hiding this comment

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

sure, we can register and we can do equals, and what happens is that the == operator of std::vector is called. This one uses the == operator on all elements, https://en.cppreference.com/w/cpp/container/vector/operator_cmp

It will not work recursively, i.e. std::vector<SGVector> will also use the == operator of the elements, which in shogun is pointer equality and not .equals

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's what I wrote above ;)

Copy link
Member

@karlnapf karlnapf left a comment

Choose a reason for hiding this comment

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

.

@karlnapf
Copy link
Member

karlnapf commented Sep 8, 2018

Could this work for any stl collection?

for (auto l = lhs.cbegin(), r = rhs.cbegin(); l != lhs.cend();
++l, ++r)
{
if (!compare(*l, *r))
Copy link
Member

Choose a reason for hiding this comment

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

Sweet

@lisitsyn
Copy link
Member Author

lisitsyn commented Sep 8, 2018

Probably, but I am not sure we need it, map/unordered_map is especially tricky to handle.

@karlnapf
Copy link
Member

karlnapf commented Sep 8, 2018

Discussed this quite a bit with @vigsterkr and we think that we do need vector, list, map, set (at least). But set list and vector would be a great start if map is hard

std::vector<CSGObject*> rhs{rhs_obj.get()};
Any any_lhs = make_any(lhs);
Any any_rhs = make_any(rhs);
EXPECT_EQ(any_lhs, any_rhs);
Copy link
Member

Choose a reason for hiding this comment

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

wait, did we want == operator for Any? Maybe I forgot this .... ;)
I think we can merge and then I add a vector parameter to our big shogun class test

@karlnapf
Copy link
Member

karlnapf commented Sep 8, 2018

@karlnapf
Copy link
Member

Soooo. what is the latest here? :)

@vigsterkr
Copy link
Member

@lisitsyn @karlnapf can i cherry-pick rebase and merge this into develop?

@karlnapf
Copy link
Member

karlnapf commented Mar 8, 2019

Yes go for it. Can be extended later on but then we at least have vector

@stale
Copy link

stale bot commented Feb 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 26, 2020
@stale
Copy link

stale bot commented Mar 4, 2020

This issue is now being closed due to a lack of activity. Feel free to reopen it.

@stale stale bot closed this Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants