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

From: David Bien (davidbien_at_[hidden])
Date: 2025-05-23 01:13:16

I noticed the specialized emplace() right away - it looked weird so I thought about it for a bit:

  template<typename... Args>
  BOOST_FORCEINLINE void emplace(Args&&... args)
  {
    insert(detail::allocator_constructed<allocator_type,value_type>{
      get_allocator(),std::forward<Args>(args)...}.value());
  }
  template<
    typename U,
    typename std::enable_if<
      std::is_same<T,detail::remove_cvref_t<U>>::value>::type* =nullptr
  >
  BOOST_FORCEINLINE void emplace(U&& x)
  {
    insert(x); /* avoid value_type construction */
  }

Given
T t;
So, the emplace( std::move(t)) will call emplace(U&&), which doesn’t move anything out of the value x, cuz insert(const&).
emplace(t) will call the emplace(Args&&… args) because emplace(U&&) can’t accept a const T& - and will make a copy of t before calling insert().

I get why it was done, but I find it to be obfuscating or confusing.
You could as well just declare the emplace for T as emplace(const U&) (which also looks weird) and a copy wouldn’t be made but it will still accept the call emplace(std::move(t)).

I think ridding emplace entirely is probably the best move - we aren’t emplacing anything in reality.

That’s my two cents, and also not really a review since I didn’t spend enough time for a real review. Hopefully I’m not annoying anyone - lol.

bien

From: Boost <boost-bounces_at_[hidden]> on behalf of Seth via Boost <boost_at_[hidden]>
Date: Thursday, May 22, 2025 at 5:24 PM
To: Boost List <boost_at_[hidden]>
Cc: Seth <bugs_at_[hidden]>
Subject: Re: [boost] [Bloom] review starts on 13th of May

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)

_______________________________________________
Unsubscribe & other changes: https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.boost.org%2Fmailman%2Flistinfo.cgi%2Fboost&data=05%7C02%7C%7Cd0e8f523d8b7452cd41e08dd9987dbce%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638835530981409230%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=GH7C0HaIzqVaL4PkxhcsvNBGjiYlnDpYtJtTxPwJXjU%3D&reserved=0<http://lists.boost.org/mailman/listinfo.cgi/boost>