mysql icon indicating copy to clipboard operation
mysql copied to clipboard

Value class re-design

Open anarthal opened this issue 2 years ago • 4 comments

The value class currently doesn't own the memory it points to, in the case of string values. This causes confusion and is error prone, and may not work well with some better buffering strategies. Make the value class own its memory.

As part of this redesign, we should also change how value::get, value::get_optional and value::get_std_optional work. With owning values, calling this functions would end up copying the string, which is undesirable. We may get rid of conversions at all and provide something similar to variant2::get_if.

Exposing the underlying variant may also be problematic. Consider removing this function.

Handling NULL values and type mismatches the same in get_optional also causes confusion. A user proposed enabling get<optional<T>>() as a way to specify "I'm trying to access a value of type T which may be NULL.

Users find the behavior for relational operators, specially operator< and friends, confusing.

anarthal avatar May 15 '22 13:05 anarthal

I don't like exposing the variant, so +1 to removing it from the public interface. In fact I would just use a union and manually manage it, because variant2 has a lot of header material to handle noexcept propagation and all that metaprogramming which just isn't needed here.

There is nothing wrong in principle with value having reference semantics and not owning the storage for strings. However, the user needs to be able to construct something which does have ownership. Given:

value v = f(...); // non-owning strings

A user should be able to take ownership by constructing a new type provided by the library, for example:

owning_value vv( v ); // owning strings

Where owning_value is a library-provided type. The idiom for this is to use the container's nested reference and value_type types.

vinniefalco avatar May 15 '22 16:05 vinniefalco

Users have mentioned that having it called value and be a view may be confusing. If we keep it a view, consider naming it sql_value_view or field_view.

anarthal avatar May 18 '22 23:05 anarthal

Yes that's good advice. field and field_view?

vinniefalco avatar May 19 '22 01:05 vinniefalco

Should work

anarthal avatar May 19 '22 10:05 anarthal