Transparent std::optional

[fallenworld1]

1. recognize std::optional in params::append and append its value(improves std::optional handling)
2. copy values for containers passed by temporary value

[fallenworld1]

I haven't read any guidelines about commit message(if there is any). If something is wrong i'm willing to fix it ofcourse

[jtv]

Sorry for not seeing this work earlier @fallenworld1. Don't worry too much about the commit messages; just "short line followed by blank line and explanation" should do.

For now it looks like tests are currently failing because of this warning:

test/test_params.cxx:34:20: error: unused variable 'text' [-Werror,-Wunused-variable]
   34 |     constexpr auto text = static_cast<int>(pqxx::format::text);

But first I'd like to understand the patch a bit better. It does look as if params fails to recognise when a std::optional contains a binary type, so that's a bug. But is this the right fix? For instance, the exact same problem will happen if the parameter is not a std::optional but a std::shared_ptr, and this approach does not fix that. AFAICS it's the binary nature of the parameter that causes the problem, when combined with any kind of extra level of indirection that append() must peel off.

So let's focus on that. The nice-to-have changes might be better in a separate PR, so we can review and merge those independently. Or I can make the changes directly on my end, just to get them out of the way. (Less work for you, but does rob you of some credits in git.) I'm not sure the change in void pqxx::params::append(params const &, sl) & is worth having, but the one in void pqxx::params::append(params &&, sl) & looks nice.

Finally, a personal code style preference: in libpqxx I don't nest namespaces like....

namespace outer
{
    // ...
    namespace inner
    {
        // ...
    }
}

I feel that's too easy to get mixed up. Instead, I write them as separate namespaces:

namespace outer
{
    // ...
} // namespace outer

namespace outer::inner
{
    // ...
}

[jtv]

To make my earlier note a bit clearer: params already handles std::optional transparently, but you've discovered that the transparency mechanism doesn't handle binary data correctly. It fails to pass on the fact that the data is binary. OTOH std::optional<int> does work, and is perfectly transparent.

Fixing it properly could be a bit more complicated.

[fallenworld1]

I don't think we should handle all possible containers. std::uniquer_ptr, std::shared_ptr and std::optional should cover 99% of cases. Also user can always extend by making a simple wrapper for params.
Meantime I can add overrides for those 3

[jtv]

We should definitely address the real problem though, not patch it over for select types.

[fallenworld1]

I'm not sure generic approach exists. What it may look like:

namespace pqxx::detail {
template<typename E> inline constexpr bool is_optional_v = false;
template<typename E> inline constexpr bool is_optional_v<::std::optional<E>> = true;

template<typename E> inline constexpr bool is_smart_ptr_v = false;
template<typename E> inline constexpr bool is_smart_ptr_v<::std::unique_ptr<E>> = true;
template<typename E> inline constexpr bool is_smart_ptr_v<::std::shared_ptr<E>> = true;
}

namespace pqxx {
template<class T>
struct nested_traits
{
    static void extract([[maybe_unused]] T &&value) { }
};

template<class T>
concept optional = detail::is_optional_v<T>;
template<optional T>
struct nested_traits<T>
{
    static auto &extract(const T& value) { return value.value(); }
};

template<class T>
concept smart_pointer = detail::is_smart_ptr_v<T>;
template<smart_pointer T>
struct nested_traits<T>
{
    static auto &extract(const T& value) { return *value; }
};

} // namespace pqxx


template<typename T>
concept nested = !std::is_same_v<decltype(pqxx::nested_traits<T>::extract(std::declval<T>())), void>;

template<nested T>void append(T &&value, sl loc = sl::current()) &
{
    if (is_null(value))
    {
      m_params.emplace_back();
    }
    else 
    {
       append(pqxx::nested_traits<T>::extract(std::forward<T>(value)), loc);
     }
}

user can extend by specifying nested_traits

[jtv]

Actually, I think there may be a simpler solution!

In params::append(TYPE &const, sl), the if constexpr / else if / else needs an additional clause: "if this is a binary value." We already have a function that can detect that recursively, seeing through any number of std::optional and std::shared_ptr and std::unique_ptr layers.

So we'd do...

if constexpr (pqxx::always_null<TYPE>())
{
    // ...
}
else if (is_null(value))
{
    // ...
}
else if (param_format(value) == format::binary)
{
    // THIS PART IS NEW
    // "There are separate binary overloads, but nevertheless this can happen
    //  if a binary param doesn't look like a binary because it's wrapped inside
    // a std::optional or a smart pointer."
}
else
{
    // ...
}

What would the new clause do? Two things, essentially:

1. Recursively dereference value until we get the actual binary value that's inside, even if it's std::optional<std::shared_ptr<std::optional<std::unique_ptr<std::optional<bytes>>>>> or something. It'd be nice to have that reasonably generic but I think for now it could be as simple as calling unary operator * until we're there.
2. Append that value.

[jtv]

I don't think we have any helpers yet for dereferencing a std::optional or smart pointer, let alone a recursive one.

It does probably make sense to have a trait to identify types like std::optional and std::shared_ptr etc. as "wrapping" a value, but then we may also need that trait to provide a generalised interface for dereferencing the wrapped value. It'd be nicely uniform for std::optional, std::shared_ptr, and std::unique_ptr, but we may also need to support std::variant and that's where it gets really different.

[fallenworld1]

I've ran into a problem with arguments lifetime.

pqxx::params p;
p.append(std::something<binary>{value});

// or something more real
class Message {
std::optional<std::vector<std::byte>> get_payload()...
};

tx.exec(params{message.get_payload()});

is going to append bytes_view creating a very easy path to store a dangling value there, not sure how to notify user about this.
I think that the most easier way is to declare that params doesn't extend lifetime of any value

[jtv]

There is one standard concept that may help: borrowed_range.

I'm also exploring a solution for the main problem by the way. It involves introducing a new traits type for such things as std::optional, std::shared_ptr, etc. It generalises the whole notion that a type wraps a value of some other type.

One of the great things about this is that it may make string_traits and nullness unnecessary for such types! For example, is_null(std::optional<int> v) will return "v has a value and is_null(v) is false," and to_string(std::optional<int> v) will return to_string(v.value()).

And, crucially, param_format(std::optional<TYPE> v) will delegate to param_format(v.value()). So recognition of binary parameters will be transparent.