insert(value_type const& x).second is always true

[cnettel]

A proper set<> will return true only if the element did not already exist. The current implementation always returns true.

The fix to check whether the bit is already set seems trivial, but I suppose that might incur some performance penalty unless the compiler will be able to conclude that the return value is not used (which is, arguably, the most frequent case).

[rhalbersma]

According to https://en.cppreference.com/w/cpp/container/set/insert:

Returns a pair consisting of an iterator to the inserted element (or to the element that prevented the insertion) and a bool value set to true if the insertion took place.

std::set<> will first check if the element to be inserted is already present, and only then actually do the insert (which has a node allocation cost). In xstd::bit_set<>, we can do an unconditonal bitwise-OR when inserting without affecting the state of the container. And therefore returning an unconditional true as the second pair member is actually what is happening.

[cnettel]

I understand that interpretation, but it is quite common to have code considering the return value of insert to reflect whether a previous call to be the negation of whether a previous call to contains would have returned true or not. Whether an insert is successful when the unconditional bitwise-OR ends up not affecting anything in the representation is then a matter for some debate. The return value is not used as an error reporting mechanism for e.g. failed memory allocations, those will be thrown instead.

I don't have access to the standard right now, but I think this behavior would violate the actual wording of the standard. From a more pragmatic standpoint, it is a possible source of bugs when trying to use bit_set as a dropin replacement for set.

I understand your design choice, though and still appreciate your library. Except for this detail, it suited my needs quite well. I was mostly surprised when I realized after a while why my code produced incorrect results. The hassle mostly arises from the fact that I tried to maintain one codepath that could be compiled with bit_set if present and set otherwise, so doing the explicit check for the element being present beforehand would incur a cost when an ordinary set is used.

[rhalbersma]

Would it help if I added an unconditional_insert member that does what the current insert does? And then add presence-checking for insert?

[cnettel]

That would make sense to me, making the identically named one behaving in an identical way to a conventional set, but keeping ready access to a shortcut version doing a plain insert.

[rhalbersma]

Finally got around looking at this. You are right: the Standard mandates returning the presence prior to insertion. Also erase() requires the return of the presence prior to erasure. I will add Pythonesque named add() and pop() members with the current behavior that return *this to allow chaining, and fix insert and erase.

[rhalbersma]

I fixed insert and erase to fully conform to the C++20 Standard and added the appropriate tests. Could you check if your use case is now working as desired? Note that you can use add() and pop() to insert and erase elements using member function chaining, equivalent to std::bitset's set and reset.

[rhalbersma]

Should be fixed now, If not, I'll re-open.