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

Protect against overloaded operator& #662

Closed
wants to merge 3 commits into from
Closed

Protect against overloaded operator& #662

wants to merge 3 commits into from

Conversation

LYP951018
Copy link

Fix #661 .

@neilmacintosh
Copy link
Contributor

@LYP951018, I was wondering if you could add some unit tests that demonstrate an overloaded operator& that causes a problem with the old implementation and then fails to disrupt the implementation with your changes.

@xaxxon
Copy link
Contributor

xaxxon commented Apr 17, 2018

I'm curious of a valid use for overloading operator& and how this should behave in that situation. Obviously you could write a bad operator& that would do the wrong thing and write a unit test against it, but op& isn't a very common thing to override so we should make sure we're understanding that use case and not breaking it before making a change that cannot be undone without breaking existing users.

@LYP951018
Copy link
Author

@neilmacintosh Unit tests have been added.

neilmacintosh pushed a commit that referenced this pull request Aug 20, 2018
* Changed &arr[0] to std::array<T, N>::data and std::address_of to protect against overloaded operator&.

* Removed the usage of `std::addressof` because it is a C++ 17 feature. Using decay for C arrays instead.

* Add unit tests for #662.

* Added c++17 test configurations for clang5.0 and clang6.0

* fixed CppCoreCheck pointer decay warning
@annagrin
Copy link

Thanks @LYP951018, your change is included #723, appreciate your help!

@annagrin annagrin closed this Apr 11, 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.

4 participants