Skip to content

Use new sets everywhere #5514

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

Merged
merged 1 commit into from
Apr 23, 2025
Merged

Use new sets everywhere #5514

merged 1 commit into from
Apr 23, 2025

Conversation

nickva
Copy link
Contributor

@nickva nickva commented Apr 23, 2025

With OTP 28 sets switched to using version 2 by default. That caused various tests to fail, as Jan discovered in #5507

In the previous PR #5510 we switched few mango instances to use a helper utility, but it was repeated a few times and Gábor made a good suggestion to use utility functions instead, so that's what this PR does. But it does it for the whole code base.

Once we're post OTP 28 we can remove the utility and just use plain set functions.

With OTP 28 sets switchd to using version 2 by default. That caused various
tests to fail, as Jan discovered in #5507

In the previous PR #5510 we switched few mango instances to use a helper
utility, but it was repeated a few times and Gábor made a good suggestion to
use utility functions instead, so that's what this PR does. But it does it for
the whole code base.

Once we're post OTP 28 we can remove the utility and just use plain set
functions.
@nickva nickva force-pushed the improve-new-sets branch from daa3a59 to 70b82a7 Compare April 23, 2025 18:11
Copy link
Contributor

@jaydoane jaydoane left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

% use version 2, which is also more performant
%
% Remove after OTP >= 28 and switch to plain sets:new/0 and sets:from_list/1

Copy link
Contributor

Choose a reason for hiding this comment

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

You could DRY this out a little:

Suggested change
-define(V2, {version, 2}).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could work, but it's only in two places and they are next to each other. The rest of the code now uses the util functions.

I debated initially using just macros (?NEW_SET() and ?SET_FROM_LIST() that would mean having to use the include couch_db.hrl in a few more places so settled on a plain utility function.

Copy link
Contributor

@pgj pgj left a comment

Choose a reason for hiding this comment

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

+1 if CI passes

@nickva nickva merged commit f012e63 into main Apr 23, 2025
23 checks passed
@nickva nickva deleted the improve-new-sets branch April 23, 2025 18:58
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.

3 participants