Remove obsolete workaround for stdlib shuffle bug #44121
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This workaround was added by #27165 The need for it was removed by catchorg/Catch2#1908 and the import of the new code.
kevingranade
commented
Sep 11, 2020
|
Sweet, this works, but I really want to clean up my commit history, hence putting it back in draft. |
kevingranade
commented
Sep 12, 2020
kevingranade
force-pushed
the
kevingranade-remove-stdlib-shuffle-bug-workaround
branch
from
a95e973 to
0c7bdf8
Compare
AMurkin
reviewed
Sep 12, 2020
src/mutation.cpp
Outdated
| @@ -130,16 +130,19 @@ void Character::toggle_trait( const trait_id &trait_ ) | |||
| const trait_id trait = trait_; | |||
| const auto titer = my_traits.find( trait ); | |||
| const auto miter = my_mutations.find( trait ); | |||
| if( titer == my_traits.end() ) { | |||
| const bool found_in_traits = titer == my_traits.end(); | |||
There was a problem hiding this comment.
Suggested change
| const bool found_in_traits = titer == my_traits.end(); | |
| const bool not_found_in_traits = titer == my_traits.end(); |
src/mutation.cpp
Outdated
| @@ -130,16 +130,19 @@ void Character::toggle_trait( const trait_id &trait_ ) | |||
| const trait_id trait = trait_; | |||
| const auto titer = my_traits.find( trait ); | |||
| const auto miter = my_mutations.find( trait ); | |||
| if( titer == my_traits.end() ) { | |||
| const bool found_in_traits = titer == my_traits.end(); | |||
| const bool found_in_mutations = miter == my_mutations.end(); | |||
There was a problem hiding this comment.
Suggested change
| const bool found_in_mutations = miter == my_mutations.end(); | |
| const bool not_found_in_mutations = miter == my_mutations.end(); |
src/mutation.cpp
Outdated
| if( titer == my_traits.end() ) { | ||
| const bool found_in_traits = titer == my_traits.end(); | ||
| const bool found_in_mutations = miter == my_mutations.end(); | ||
| if( found_in_traits ) { |
There was a problem hiding this comment.
Suggested change
| if( found_in_traits ) { | |
| if( not_found_in_traits ) { |
src/mutation.cpp
Outdated
| my_traits.insert( trait ); | ||
| } else { | ||
| my_traits.erase( titer ); | ||
| } | ||
| if( ( titer == my_traits.end() ) != ( miter == my_mutations.end() ) ) { | ||
| // Checking this after toggling my_traits, if we exit the two are now consistent. | ||
| if( found_in_traits != found_in_mutations ) { |
There was a problem hiding this comment.
Suggested change
| if( found_in_traits != found_in_mutations ) { | |
| if( not_found_in_traits != not_found_in_mutations ) { |
src/mutation.cpp
Outdated
| debugmsg( "my_traits and my_mutations were out of sync for %s\n", trait.str() ); | ||
| return; | ||
| } | ||
| if( miter == my_mutations.end() ) { | ||
| if( found_in_mutations ) { |
There was a problem hiding this comment.
Suggested change
| if( found_in_mutations ) { | |
| if( not_found_in_mutations ) { |
|
Aha you're totally right, I'm going to apply your change manually to avoid extra commits. |
GLIBCXX_DEBUG was complaining about comparing titer to my_traits.end() after calling my_traits.erase( titer ), as titer is invalidated.
kevingranade
force-pushed
the
kevingranade-remove-stdlib-shuffle-bug-workaround
branch
from
0c7bdf8 to
dc2fa7e
Compare
ZhilkinSerg
added
[C++]
kevingranade
deleted the
kevingranade-remove-stdlib-shuffle-bug-workaround
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
SUMMARY: None
Purpose of change
Now that the main block of Travis tests is passing again, the "try many variations" tests are running again, and some of them are failing. In particular the test that specifies _GLIBCXX_DEBUG is throwing a warning about an unused macro.
After clearing compilaion, GLIBCXX_DEBUG does what it's supposed to and reports invalid iterator handling, yay.
Describe the solution
I started to just silence the macro, but then realized that it might be a valid warning. Upon digging into the reason for the workaround and what has happened since, it looks like the reason for the workaround has been removed.
Also address the invalid pointer handling.
Describe alternatives you've considered
Silencing the error, but I think it's a real report.
Testing
Building would be nice.
The way we invoke catch in Travis exercises this feature, so a clean run of the failing test should be sufficient.
Additional context
I'm adding the test higher up i the travis build stages to exercise it more quickly.
This workaround was added by #27165
The need for it was removed by catchorg/Catch2#1908 and the import of the new code.