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

Add missing erase(first, last) function to ordered_map #3109

Merged
merged 7 commits into from
Nov 9, 2021

Conversation

nlohmann
Copy link
Owner

Fixes #3108

@nlohmann nlohmann added the review needed It would be great if someone could review the proposed changes. label Oct 30, 2021
@nlohmann nlohmann added this to the Release 3.10.5 milestone Oct 30, 2021
@nlohmann nlohmann self-assigned this Oct 30, 2021
@coveralls
Copy link

coveralls commented Oct 30, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling 80067d9 on issue3108_erase_in_ordered_map into 7440786 on develop.

@nlohmann nlohmann added the state: help needed the issue needs help to proceed label Oct 30, 2021
@nlohmann
Copy link
Owner Author

There is an assertion that only hits on MSVC builds:

C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\vector(199) : Assertion failed: vector iterators incompatible

As I do not work on Windows, I cannot debug into this problem to get a stack trace. Any help is greatly appreciated!

@nlohmann nlohmann mentioned this pull request Oct 31, 2021
@ltjax
Copy link

ltjax commented Nov 8, 2021

>	test-ordered_map.exe!std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<std::pair<std::string const ,std::string>>>>::_Compat(const std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<std::pair<std::string const ,std::string>>>> & _Right) Line 190	C++
 	test-ordered_map.exe!std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<std::pair<std::string const ,std::string>>>>::operator==(const std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<std::pair<std::string const ,std::string>>>> & _Right) Line 154	C++
 	test-ordered_map.exe!doctest::detail::Expression_lhs<std::_Vector_iterator<std::_Vector_val<std::_Simple_types<std::pair<std::string const ,std::string>>>> &>::operator==<std::_Vector_iterator<std::_Vector_val<std::_Simple_types<std::pair<std::string const ,std::string>>>>>(std::_Vector_iterator<std::_Vector_val<std::_Simple_types<std::pair<std::string const ,std::string>>>> && rhs) Line 1238	C++
 	test-ordered_map.exe!_DOCTEST_ANON_FUNC_2() Line 248	C++
 	test-ordered_map.exe!doctest::Context::run() Line 6487	C++
 	test-ordered_map.exe!main(int argc, char * * argv) Line 6571	C++

Here's the call-stack

@ltjax
Copy link

ltjax commented Nov 8, 2021

VS seems to be in line with cppreference, at least.
See https://en.cppreference.com/w/cpp/container/vector/pop_back: Iterators and references to the last element, as well as the end() iterator, are invalidated.
The Container::resize at the end of ordered_map::erase is equivalent to a N pop_back()s, and that the last one of those invalidates your 'first'.

@gregmarr
Copy link
Contributor

gregmarr commented Nov 8, 2021

Seems like if the range being removed is at the end, that requires resetting the first iterator to be end() before returning.

@ltjax
Copy link

ltjax commented Nov 8, 2021

I added auto offset = std::distance(Container::begin(), first); at the beginning of the function and returned return Container::begin() + offset; at the end. That fixes it as well.

@nlohmann
Copy link
Owner Author

nlohmann commented Nov 9, 2021

Thanks for the help! I will update the PR later on. Very interesting to see that only MSVC debug mode detected this issue!

@gregmarr
Copy link
Contributor

gregmarr commented Nov 9, 2021

Thanks for the help! I will update the PR later on. Very interesting to see that only MSVC debug mode detected this issue!

I think that this is solely because of the extra debugging information that MSVC debug adds. The first iterator at the end of the operation should be exactly the same value as you get by begin() + count when the iterators are just pointers into the vector, which they are most of the time. There is no observable difference between the old and new code without that extra debugging, but the new code is more correct.

@nlohmann
Copy link
Owner Author

nlohmann commented Nov 9, 2021

Thanks for the help! I will update the PR later on. Very interesting to see that only MSVC debug mode detected this issue!

I think that this is solely because of the extra debugging information that MSVC debug adds. The first iterator at the end of the operation should be exactly the same value as you get by begin() + count when the iterators are just pointers into the vector, which they are most of the time. There is no observable difference between the old and new code without that extra debugging, but the new code is more correct.

Yes, I'm aware that this is really pedantic - that's why I would have expected UBSAN or ASAN to detect this.

Anyway - thanks @ltjax and @gregmarr for the help!

@nlohmann nlohmann added release item: 🐛 bug fix and removed state: help needed the issue needs help to proceed review needed It would be great if someone could review the proposed changes. labels Nov 9, 2021
@nlohmann nlohmann merged commit e9f88c2 into develop Nov 9, 2021
@nlohmann nlohmann deleted the issue3108_erase_in_ordered_map branch November 9, 2021 21:24
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.

ordered_json doesn't support range based erase
4 participants