ArthurSonzogni / FTXUI

:computer: C++ Functional Terminal User Interface. :heart:
MIT License
6.9k stars 415 forks source link

ref.hpp #671

Open ProkopRandacek opened 1 year ago

ProkopRandacek commented 1 year ago

Hi :D

what is the reasoning behind the types in ref.hpp? I am struggling to understand what purpouse they serve.

How do you manage the lifetime of StringRef's address_? I don't understand why FTXUI components don't rather take ownership over those string by just using std::string instead of StringRef, ConstStringRef or ConstStringListRef.

One scenario that comes to mind is when StringRef is constructed from a std::string*, there is no guarantee that this pointed-to std::string will not be destroyed by its owner.

I have not checked everything but i believe that for example Button.label_'s lifetime is tied to the lifetime of the Button and thus should be std::string member variable, rather than "maybe member variable, maybe raw pointer"

I have similar concerns about Ref and ConstRef.

Thanks for clarifications!

ArthurSonzogni commented 1 year ago

Hello!

Given:

So the solution I found is "Ref". A Ref is either a value or a pointer to an external value. Developers can provide the kind they want.

So either an "owned" or an "unowned" value.

The Const-xxx variant means the component can't modify the value.

The xxx-String variant was useful during the wstring -> string transition, so that users can continue to use any of the string type in the same component.

Does this help?

ProkopRandacek commented 1 year ago

Thank you for your reply!

On Wed Jun 7, 2023 at 11:15 PM CEST, Arthur Sonzogni wrote:

  • I would like some properties to be bound to two two components possibly. For instance, the selected entry of an horizontal menu could be index of the "tab" to display. For this to work we could say a "property" is a pointer to an "external" variable. With a single source of truth, there are no need to "synchronize" things.

That makes sence. But you are risking a use-after-free when the owning component dies but the refering component is still alive. I think std::shared_ptr would be a safer and more well known solution. Alternatively i could see just taking a T& everywhere and letting the user manage the lifetime of shared state manually.

  • I would also like developers not being required to specify every properties. I would like them to be able to rely on some "default" value, or even specify there own constant.

I believe that this could be solved in any case with function overloading.

The Const-xxx variant means the component can't modify the value.

This one I find confusing. If you are can't modify the value, then sharing it across components doesn't make sence, right? This one I suspect shoule be just passed by value. Or do you mean that the const-ness only applies for some components but not for others? In that case I believe you could again use std::shared_ptr and give a const T version to the readonly component and T version to the read/write component.

The xxx-String variant was useful during the wstring -> string transition, so that users can continue to use any of the string type in the same component.

That makes sense

ArthurSonzogni commented 1 year ago

That makes sence. But you are risking a use-after-free when the owning component dies but the refering component is still alive.

Most developers are going to use shared_ptr.

However, I wouldn't object adding support for passing shared_ptr in addition. to ptr and values, or some way to support arbitrary data type supporting some kind of interface.

ProkopRandacek commented 1 year ago

I looked at the ref.hpp file again and I have noticed something weird: If I pass const std::wstring* to the ConstStringRef constructor, it is going to create a copy of it and not share the state with whoever owns the original std::wstring, which I thought was the reason for having this special reference type. Am I missunderstadning something?

Thanks!

ProkopRandacek commented 1 year ago

I also wonder if it would be possible to always take by value and use std::ref to achieve shared state between components. But I am not sure if I understand std::ref correctly

cculianu commented 4 months ago

FWIW I am a seasoned C++ dev here and I have no criticisms of your design/thinking behind the Ref type. I think it's appropriate and a clever solution to sometimes-ownership sometimes-not ownership. And like you say, the key thing it brings to the table is the ability for interoperating components to always "see" the same exact property. This is huge. If you were to not go with this approach a lot of boilerplate code would have to be written to keep inter-oping components talking to each other to synch up properties and it would just get potentially ugly quite quickly.

One could have used a shared_ptr here but this feels like overkill for something small like a bool property when just careful usage of the type would suffice.

So, with the lightweight choise of design.. it's true that use-after-free is now possible if one mis-uses this type, that is the nature of the beast with C++ -- you can sometimes sacrifice "fool proof-ness" for speed/simplicity/efficiency. Think about the C++17 std::string_view and C++20 std::span: both of these types are non-owning references that don't ever do anything about keeping their referred-to objects alive.. and use-after-free is certainly possible with them.. they can be misused as well but the language committee decided that having them exist is worthwhile for many situations.

Just saying...

cculianu commented 4 months ago

I also wonder if it would be possible to always take by value and use std::ref to achieve shared state between components. But I am not sure if I understand std::ref correctly

std::ref (or, rather std::reference_wrapper) is implemented as a pointer. It's just a wrapper for a pointer but it has "reference semantics" when used in user code. You can use it for templates or other code not expecting a pointer .. to give it a pointer instead of a real value.. It... really brings nothing to the table here... that a pointer doesn't already do.