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 BuildNumberSpec #3098

Merged
merged 5 commits into from
Jan 4, 2024
Merged

Add BuildNumberSpec #3098

merged 5 commits into from
Jan 4, 2024

Conversation

AntoinePrv
Copy link
Member

@AntoinePrv AntoinePrv commented Jan 3, 2024

No description provided.

{
return (equal_with<T>(lhs, rhs) || ...);
}
}
Copy link
Member

@JohanMabille JohanMabille Jan 3, 2024

Choose a reason for hiding this comment

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

I don't get the need for all this complexity. From what I understand (but I might be wrong), you want to ensure that the variant hold objects of the same type, which you can check with lhs.index() == rhs.index(). With this, you don't need all the equal overloads at all.

Copy link
Member Author

@AntoinePrv AntoinePrv Jan 3, 2024

Choose a reason for hiding this comment

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

No, I want to ensure they contains the same index and that the contained objects compare equal.
I cannot do it with operator== because that would mean specializing in namespace std.

Copy link
Member

@JohanMabille JohanMabille Jan 3, 2024

Choose a reason for hiding this comment

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

From the definitions I read here, it looks like objects are considered equal if they have the same type. Did I miss something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha I see what you mean. The code written does a proper variant comparison, but from the types included here (none have members), it is equivalent to only checking for type equality.
This is not what I wrote because this does not describe the proper intent. It may lead to subtle bugs should one modify/add a predicate.


auto operator==(const BuildNumberPredicate& lhs, const BuildNumberPredicate& rhs) -> bool
{
return (lhs.m_build_number == rhs.m_build_number) && equal(lhs.m_operator, rhs.m_operator);
Copy link
Member

@JohanMabille JohanMabille Jan 3, 2024

Choose a reason for hiding this comment

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

Actually you could even perform the variant comparison here and get rid of the auto equal(const std::variant<T...>& lhs, const std::variant<T...>& rhs) function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, I cannot specialized operator==(std::equal_to<int>, std::equal_to<int>) without UB.

Copy link
Member

@JohanMabille JohanMabille Jan 3, 2024

Choose a reason for hiding this comment

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

Is there a case where operator==(std::equal_to<int>, std::equal_to<int>) (or any overload) should return false? Otherwise, comparing the type is enough.

@AntoinePrv AntoinePrv merged commit 2763214 into mamba-org:main Jan 4, 2024
27 checks passed
@AntoinePrv AntoinePrv deleted the matchspec branch January 4, 2024 10:57
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.

2 participants