Re: [Bloom] review starts on 13th of May

From: Seth (bugs_at_[hidden])
Date: 2025-05-22 23:24:23

On Thu, May 22, 2025, at 7:09 PM, Joaquin M López Muñoz via Boost wrote:
> BTW, emplace() does construct the element, calculates its hash, and then 
> throws
> it away.

In other words, it *doesn't emplace*. 

"emplace" to me always meant "inplace-construction" (for node-based containers, std::optional, std::any, std::variant). If `emplace` by definition destructs the element constructed, should it have a different name?

I think I agree with those that think the adherence to container-like member names does more harm than good. `insert` is fine, because in _some sense_ (potential set membership) an element is being inserted. For the current semantics, `emplace` should just something like `insert_from_temporary`, but I don't see the appeal over `insert(T(...))`.

The uses-allocator argument just highlights to me, that having the allocator be `allocator<T>` instead of `allocator<unsigned char>` feels misleading to begin with. (There might be advanced scenarios that I missed reading of the docs, but my understanding is that bloom::filter in principle never stores T?).

In this light, I prefer the caller be in charge of any allocators used for temporaries of T. It's actually more likely that it would *not* coincide with the allocator for the filter array (e.g. a stack/pool allocator for repeated temps; bloom filters themselves typically have longer lifetimes and fixed capacities, the polar opposite usage pattern).

Also, having the temporary right there in the interface (`insert(T(....))`) makes the [allocation] cost visible to the user. It might prompt to the user to switch to a custom `Hash` type that doesn't incur the [same] cost.

Hopefully my understanding doesn't completely miss the mark - in which case perhaps it could spark more ideas for documentation improvements :)

Seth

(PS. This is not a review, because I didn't spend enough time)