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

Create comparator wrapper classes for MSVC 2019 build issues: #3786

Closed
wants to merge 1 commit into from

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Mar 5, 2021

High Level Overview of Change

Travis CI builds started failing for Visual Studio 2019 builds. Example 1, example 2.

@scottschurr , @miguelportilla , and I tracked the problem down to a recent update to VS 2019. The unaltered current develop branch fails with this version, so it is definitely not due to changes in rippled code.

The underlying cause is that the std::less and std::equal_to comparators have added the [[nodiscard]] attribute, which conflicts with boost::bimap type validations, which call the functions without capturing the result.

This fix adds classes to exactly wrap the comparators, removing [[nodiscard]] attributes.

Type of Change

  • [X ] Refactor (non-breaking change that only restructures code)

Future Tasks

The new wrapper classes can be removed when either:

  • A Boost update fixes this issue.
  • A VS 2019 update fixes this issue.

This change is Reviewable

@@ -21,6 +21,7 @@

#include <ripple/basics/UnorderedContainers.h>
#include <ripple/basics/chrono.h>
#include <ripple/basics/comparitors.h>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is misspelled? "comparators.h"?

@scottschurr
Copy link
Collaborator

Also, the commit message misspells "comparators" I think. That would be good to fix.

@scottschurr scottschurr self-assigned this Mar 11, 2021
@ximinez
Copy link
Collaborator Author

ximinez commented Mar 16, 2021

Also, the commit message misspells "comparators" I think. That would be good to fix.

🤦‍♂️ Will fix.

@ximinez
Copy link
Collaborator Author

ximinez commented Mar 16, 2021

I fixed the typo in a FOLD commit, and rewrote history to fix the commit message, but didn't change any of the code in that commit.

@scottschurr
Copy link
Collaborator

👍 Looks great!

@nbougalis nbougalis added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 17, 2021
@intelliot intelliot changed the title Create comparitor wrapper classes for MSVC 2019 build issues: Create comparator wrapper classes for MSVC 2019 build issues: Mar 23, 2021
//------------------------------------------------------------------------------
/*
This file is part of rippled: https://github.com/ripple/rippled
Copyright (c) 2012, 2013 Ripple Labs Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright (c) 2021 Ripple Labs Inc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 Fixed.


} // namespace ripple

#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing carriage return

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

@miguelportilla miguelportilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

* The std::less and std::equal_to comparators have [[nodiscard]], which
  conflicts with boost::bimap validations.
@ximinez
Copy link
Collaborator Author

ximinez commented Mar 26, 2021

Squashed and rebased onto 1.8.0-b1

This was referenced Mar 31, 2021
@ximinez ximinez deleted the travis-vs2019 branch April 1, 2021 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants