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

From: Joaquin M López Muñoz (joaquinlopezmunoz_at_[hidden])
Date: 2025-05-23 09:54:31

El 23/05/2025 a las 1:24, Seth via Boost escribió:
> 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(...))`.

My personal preference is towards mimicking container APIs because this is
likely one of the most familiar interfaces out there. Alexander seems to be
on the same front. Anyway, I'll do whatever the community agrees on --or the
review manager decides. This particular member function is a very minor
detail in the grand scheme of things.

> 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?).

bloom::filter never stores T. As for having allocator<T>, again this is for
consistency with all standard usages I know of, where the default allocator
is std::allocator<value_type>. Internally, an allocator rebound to 
usigned char
is indeed used.

> 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).

Right now, it is perfectly possible (and expected) to pass T values to 
the filter
that have been constructed by the user anyway they please.

> 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 :)

I think your understanding of the matter is correct. Thanks for your 
comments!

Joaquin M Lopez Munoz