Entt: Better semantics for try_get

[sssilver]

Hi,

Presently registry::try_get() returns a pointer to an entity that is nullptr if the requested entity doesn't exist.

Since C++17 finally has optionals, its semantics would be improved if instead it returned a _reference_ to an entity, wrapped in an std::optional. This would make the interface more predictable and communicate design intent more explicitly.

Happy to open a PR for this if that'd help.

[Kerndog73]

A pointer can either be nullptr or point to an object. An std::optional can either be std::nullopt or hold a valid object. try_get with a pointer or std::optional would have the same usage syntax. std::optional just adds extra overhead. I don't see the point to be honest.

[sssilver]

While they both can be used to achieve the same effect, I believe an std::optional more explicitly conveys the meaning of "you should safely unwrap this, the nil-value should be handled properly by your business logic".

There are also more practical reasons: I have opened this ticket because I was confused about "who owns the memory pointed at by this pointer?" "Who's responsible for deallocating it?", when reviewing a certain PR that used _entt_. In case of a pointer, one must know the implementation details of the library, they must remember that in this particular case registry owns the memory and memory management shouldn't be a concern. std::optional alleviates that.

In other words, std::optional<entity> is self-documenting in a way that entity* isn't.

[Kerndog73]

try_get would have to return std::optional<Component &> so it has the same lifetime problem as a pointer except that it’s hidden in a reference in an object.

[Kerndog73]

The reference returned by get has the same lifetime of the pointer returned by try_get but you don’t seem to be complaining about that. If you’re unsure of the lifetime of a reference or pointer, you should check the documentation. Even if the reference is wrapped in a std::optional, you’d still need to read the docs if you’re unsure about the lifetime.

[Kerndog73]

I’m all for modern C++ but I’m yet to find a place where std::optional is a good fit.

[Kerndog73]

A pointer has meaning in C++. It is self documenting. A pointer is nullable. A pointer has a different meaning to a reference and this is well understood by C++ programmers.

[sssilver]

I haven't been complaining, I've been trying to improve (your?) library.

Unlike a pointer, a reference communicates an implicit contract that the caller is "borrowing" the value and shouldn't be concerned about its lifetime.

It's your call, I believe I have provided all the arguments. If you're been looking for it, this is the perfect idiomatic fit for std::optional, however if you don't see it there's little more that I can do. Perhaps other advocates of idiomatic modern C++ would be able to present the case better than I did. In either case, thanks for your work and have a good day.

[Kerndog73]

A pointer has meaning in C++. It is self documenting. A pointer is nullable. A pointer has a different meaning to a reference and this is well understood by C++ programmers. In “good code” a raw pointer never owns memory so you don’t have to deallocate it. A pointer is a nullable reference.

[skypjack]

Let's take a deeper look into the issue before to continue discussing. I think there is a basic misunderstanding in the OP's request (emphasis mine):

Since C++17 finally has optionals, its semantics would be improved if instead it returned a reference to an entity, wrapped in an std::optional.

To be clear, try_get doesn't return a reference to an entity. It wouldn't make sense, mainly because the entity is what you pass as an argument.
As get<C>(entity) returns a C & for the entity, try_get<C>(entity) returns a C * (eventually const in both cases, but we can safely ignore this part here). Therefore, std::optional<C> would be all the ways wrong.
In other words, try_get(entity) -> C * is a slightly faster alternative of the following:

if(registry.has<C>(entity)) {
    C *instance = &registry.get<C>(entity);
}

Using an std::optional<C> wouldn't preserve the semantic and it would turn it in an alternative for the following instead:

if(registry.has<C>(entity)) {
    C instance = registry.get<C>(entity);
}

The subtle difference is that you're obtaining a copy of the component, that is useless most of the times, other than a waste of CPU cycles.

That said, I think we agree on the fact that we cannot use std::optional<C>.
The other way around would be an std::optional<C &>, but this isn't allowed by the standard.
It seems that the remained solution is then to replace C * with the following:

std::optional<std::reference_wrapper<C>>

Put aside the fact that it requires to include two extra headers in registry.hpp (at the moment, neither <optional> nor <functional> are included) and ignore for a while the uglier syntax, this is what we obtain then to access our component:

struct C { int value; };
// ...
if(auto opt = registry.try_get<C>(entity); opt) {
   opt->get().value = 42;
}

That is, the combination of an std::optional and an std::reference_wrapper won't change the semantics, but it will change the syntax quite a lot if compared with the current one:

if(auto *c = registry.try_get<C>(entity); c) {
   c->value = 42;
}

To be honest, it seems less intuitive to me, at least for a C++ programmers, but this is a sort of personal opinion.

---

Just to be sure, is this what you're asking for? Are you proposing to replace the return type of try_get and to use an std::optional<std::reference_wrapper<C>> instead of a C * for a given component C?

[skypjack]

As a side question:

when reviewing a certain PR that used entt

For what are you using EnTT?
I'd be glad to put your project or your company in the _EnTT in Action_ page of the wiki if possible!!

[skypjack]

@sssilver Any feedback on the questions above?
At the moment the issue is stuck because the request seems to be based on a misunderstanding and the solution seems to be more complicated than initially assumed.

[sssilver]

@skypjack Using EnTT for a cross-platform video game. We'll be happy to credit you when the game is launched, and will definitely let you know to update your Action page.

With regards to std::optional, thank you for the comprehensive response. I think I'm convinced that my idea was faulty in the universe of C++. Sorry about the time people had to spend on this.

[skypjack]

You're welcome, thank you for pointing it out. Discussions on the library are always welcome. :+1:

[LiliumAtratum]

I accidently stumbled upon this old ticket and I understand where the author is coming from. Whenever we get a unique_ptr<T>, shared_ptr<T> or T& it is clear about the ownership of the object. However, for T* it is not, and you have to consult with the documentation (or simply know it).
At the same time, I see that using std::optional<T&> or std::optional<std::reference_wrapper<T>> is not possible or not feasable.

There was actually a discussion on optional references on this:
https://www.fluentcpp.com/2018/10/05/pros-cons-optional-references
(I am in a no-rebinding camp, btw. For me std::optional<T&> should not be rebindable at all, even if the optional is empty)

However, for entt - an alternative solution would be to fill into what the standard is missing, and implement entt::maybe_ref<T> for this, with a semantic of non-rebindable, nullable T&. Going one step further, we could also have entt::ref<T> with the semantic of T&, and attach triggering on_update on assignment (in both ref and maybe_ref types).

[skypjack]

Uhm... isn't it a bit overkilling? It introduces yet another specialization to clarify the semantic of a function return type that is imho clear enough already. I can understand that a free function T * foo() doesn't tell you much about who is in charge of freeing the resource. Though, in this case, it's a known thing that the registry owns all instances of components.

[Innokentiy-Alaytsev]

Even more: if I understand Core Guidlines, you should never use owning raw pointers in modern C++ API anyway. And if you need to you should use gsl::owner for wrapping the pointer.

[codylico]

I have opened this ticket because I was confused about "who owns the memory pointed at by this pointer?" "Who's responsible for deallocating it?", when reviewing a certain PR that used entt.

Maybe what's needed is an additional note in the method's documentation?

/**
 * \note The registry retains ownership of the returned pointer(s).
 */

[skypjack]

This is probably our best bet. Do you want to open a PR for that @codylico ? 🙂

[codylico]

Yes, I can do that. PR from which branch?

[skypjack]

@codylico sorry, sleeping time for me 8 hours ago. 🙂 I'd say branch experimental.