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

Fix operator conflict #2421

Closed
wants to merge 1 commit into from
Closed

Fix operator conflict #2421

wants to merge 1 commit into from

Conversation

Giulero
Copy link
Contributor

@Giulero Giulero commented Nov 25, 2020

This pull request should fix #2067.
cc @GiulioRomualdi @traversaro

@Giulero Giulero requested a review from drdanz as a code owner November 25, 2020 19:35
@update-docs
Copy link

update-docs bot commented Nov 25, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update the release notes by adding a file in doc/release/<target_branch>, based on your changes.

@CLAassistant
Copy link

CLAassistant commented Nov 25, 2020

CLA assistant check
All committers have signed the CLA.

@PeterBowman
Copy link
Member

Does the removal of said operator affect in any way the LogStream counterpart, i.e. is it still correctly displaying std::vectors?

template <typename T>
inline LogStream& operator<<(const std::vector<T>& t)
{
stream->oss << t;
stream->oss << ' ';
return *this;
}

@GiulioRomualdi
Copy link
Member

Hi @Giulero checking the CI seems that function you removed is widely used in yarp

@Giulero
Copy link
Contributor Author

Giulero commented Nov 26, 2020

@PeterBowman thank you for pointing this out! @GiulioRomualdi, indeed. The removal seems quite harmful.
@traversaro @drdanz how do you suggest to proceed?

@PeterBowman
Copy link
Member

I think that pasting the body of the deleted function into LogStream::operator<<(const std::vector<T> &) should make CI happy.

@drdanz
Copy link
Member

drdanz commented Dec 2, 2020

@drdanz how do you suggest to proceed?

@PeterBowman suggestion is probably the best option

@drdanz
Copy link
Member

drdanz commented Dec 2, 2020

Also I believe that instead of moving the implementation in Types.h might not be needed after moving the implementation...

@traversaro
Copy link
Member

Sorry for the late response. I totally agree with @drdanz and @PeterBowman , the problem that caused #2067 is adding new operators in the std namespace, adding new operators for the classes in YARP is totally fine.

@drdanz
Copy link
Member

drdanz commented Apr 14, 2021

Closing this in favour of #2548

drdanz added a commit to drdanz/yarp that referenced this pull request Apr 19, 2021
@drdanz drdanz closed this May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collision between YARP and Casadi
6 participants