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.

[indianakernick]

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.

[indianakernick]

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.

[indianakernick]

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.

[indianakernick]

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

[indianakernick]

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.

[indianakernick]

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

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