gb_sets and gb_trees: Add checks to from_ordset and from_orddict#10910
gb_sets and gb_trees: Add checks to from_ordset and from_orddict#10910bjorng merged 1 commit intoerlang:masterfrom
gb_sets and gb_trees: Add checks to from_ordset and from_orddict#10910Conversation
CT Test Results 2 files 100 suites 1h 3m 58s ⏱️ Results for commit 9e122bb. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts
// Erlang/OTP Github Action Bot |
0ead779 to
9c8f5b6
Compare
bjorng
left a comment
There was a problem hiding this comment.
Thanks for your pull request. It's looks good to me (except for one implementation detail), but we will have to discuss it in the OTP team.
Sure, no problem. As it turns out, this change has already unearthed a bug in |
74ff048 to
9e122bb
Compare
9e122bb to
c4d20b5
Compare
Co-authored-by: Jan Uhlig <juhlig@hnc-agency.org>
c4d20b5 to
71b86fb
Compare
|
Thanks for your pull request. |
I recently had the pleasure to debug some code where, ultimately, an unordered list and/or with duplicates, was given to
gb_sets:from_ordset/1. The implementation does not complain, but silently turns the list into an invalid gb_set, resulting in misbehaviors like elements that are in the set not being found, and elements that have been deleted from the set still being reported as being in the set. Adding or deleting elements from the set may also result in elements appearing and disappearing. And so on 🤷♀️This is a mistake easily made. Until recently, the documentation kinda encouraged the use of
from_ordsetin multiple places, with the assumption that the list given to it is sorted and free of duplicates only mentioned in the prose offrom_ordset. It is also a mistake very hard to find once made, more so when you're not the person who wrote that code, and the person who did left years ago. (😡)Anyway. This PR adds checks to the functions which turn lists-that-may-be-ordsets-or-not. Internal uses bypass the check in order to not impose a penalty when not necessary.
This also applies to
gb_trees:from_orddict/1, which is likewise changed by this PR.On the way, we also added
gb_trees:from_list/1, and removed the one remaining old-stylecatchinsets_SUITE.There are no tests yet, we would like to hear if this addition is really wanted or not before putting more work into it 😉
This change may break some existing code, but if so, IMO for the better, as it would clearly be a bug.