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

Ensure strict weak ordering in scored_address::operator>() #58248

Merged
merged 1 commit into from
Jun 9, 2022

Conversation

BrettDong
Copy link
Member

@BrettDong BrettDong commented Jun 7, 2022

Summary

None

Purpose of change

The comparator in a std::priority_queue has to obey strict weak ordering, otherwise it's going to cause undefined behaviour. The greater operator of scored_address does not meet this condition:

bool operator> ( const scored_address &other ) const {
    return score >= other.score;
}

A > B and B > A are simultaneously true when their scores are equal and this violates the strict weak ordering requirement.

This causes assertion failure and crashes the program in MSVC Debug build as well as in _GLIBCXX_DEBUG mode in GCC.

Describe the solution

Change the greater operator of scored_address to do strict greater comparison of score field.

  1. This does not seems to affect the semantic of maintaining a min heap priority queue in the A-star algorithm.
  2. The current behaviour is undefined anyway.
  3. The order of elements of equal priority in a std::priority_queue is undefined anyway.

Describe alternatives you've considered

Testing

Additional context

@BrettDong BrettDong added the <Bugfix> This is a fix for a bug (or closes open issue) label Jun 7, 2022
@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Vehicles Vehicles, parts, mechanics & interactions json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jun 7, 2022
@dseguin dseguin merged commit 3eaa7c8 into CleverRaven:master Jun 9, 2022
@BrettDong BrettDong deleted the comparator branch June 10, 2022 14:59
bombasticSlacks pushed a commit to bombasticSlacks/Cataclysm-DDA that referenced this pull request Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in test "find_overmap_path_bridge" in Debug build
2 participants