PQXX_DECLARE_ENUM_CONVERSION issues #807

alexolog · 2024-03-19T02:29:49Z

When I define an enum
enum class MyEnum : int16_t { a, b, c }; // smallint

Then declare
namespace pqxx { PQXX_DECLARE_ENUM_CONVERSION(MyEnum); }

And try to use it in a tuple, I get the following error when converting a row to the tuple:

lib/pqxx/arm64-macOS/include/pqxx/field.hxx:538:39: error: no viable conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'const std::string' (aka 'const basic_string<char>')
  538 |       internal::throw_null_conversion(type_name<T>);
      |                                       ^~~~~~~~~~~~
lib/pqxx/arm64-macOS/include/pqxx/row.hxx:567:24: note: in instantiation of function template specialization 'pqxx::from_string<MyEnum>' requested here
  567 |   std::get<index>(t) = from_string<field_type>(f);
      |                        ^
lib/pqxx/arm64-macOS/include/pqxx/row.hxx:268:6: note: in instantiation of function template specialization 'pqxx::row::extract_value<std::tuple<long long, long long, long long, MyEnum, std::string, long long>, 3UL>' requested here
  268 |     (extract_value<Tuple, indexes>(t), ...);
      |      ^
lib/pqxx/arm64-macOS/include/pqxx/row.hxx:243:5: note: in instantiation of function template specialization 'pqxx::row::extract_fields<std::tuple<long long, long long, long long, MyEnum, std::string, long long>, 0UL, 1UL, 2UL, 3UL, 4UL, 5UL>' requested here
  243 |     extract_fields(t, std::make_index_sequence<std::tuple_size_v<Tuple>>{});
      |     ^
lib/pqxx/arm64-macOS/include/pqxx/row.hxx:176:5: note: in instantiation of function template specialization 'pqxx::row::convert<std::tuple<long long, long long, long long, MyEnum, std::string, long long>>' requested here
  176 |     convert(t);
      |     ^
my_source.cpp:148:13: note: in instantiation of function template specialization 'pqxx::row::to<std::tuple<long long, long long, long long, MyEnum, std::string, long long>>' requested here
  148 |         row.to(t);
      |             ^
/opt/homebrew/opt/llvm/bin/../include/c++/v1/string:896:33: note: candidate constructor not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'const string &' for 1st argument
  896 |   _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(const basic_string& __str)
      |                                 ^            ~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/homebrew/opt/llvm/bin/../include/c++/v1/string:913:55: note: candidate constructor not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'string &&' for 1st argument
  913 |   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(basic_string&& __str)
      |                                                       ^            ~~~~~~~~~~~~~~~~~~~~
/opt/homebrew/opt/llvm/bin/../include/c++/v1/string:937:55: note: candidate constructor template not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'const char *' for 1st argument
  937 |   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(const _CharT* __s)
      |                                                       ^            ~~~~~~~~~~~~~~~~~
/opt/homebrew/opt/llvm/bin/../include/c++/v1/string:951:3: note: candidate constructor not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'nullptr_t' (aka 'std::nullptr_t') for 1st argument
  951 |   basic_string(nullptr_t) = delete;
      |   ^            ~~~~~~~~~
/opt/homebrew/opt/llvm/bin/../include/c++/v1/string:1083:55: note: candidate constructor not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'initializer_list<char>' for 1st argument
 1083 |   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(initializer_list<_CharT> __il)
      |                                                       ^            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/homebrew/opt/llvm/bin/../include/c++/v1/string:886:64: note: explicit constructor is not a candidate
  886 |   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit basic_string(const allocator_type& __a)
      |                                                                ^
/opt/homebrew/opt/llvm/bin/../include/c++/v1/string:1039:93: note: explicit constructor is not a candidate
 1039 |   _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit basic_string(const _Tp& __t)
      |                                                                                             ^
lib/pqxx/arm64-macOS/include/pqxx/internal/conversions.hxx:58:42: note: passing argument to parameter 'type' here
   58 | throw_null_conversion(std::string const &type);
      |                                          ^

The text was updated successfully, but these errors were encountered:

tt4g · 2024-03-19T03:49:12Z

The conversion from rvalue to lvalue is probably failing because it is trying to pass throw_null_conversion(std::string const &)　std::string_view returned by type_name<T>().

libpqxx/src/strconv.cxx

Lines 253 to 256 in 9d2a459

    
           void PQXX_COLD throw_null_conversion(std::string const &type) 
        
           { 
        
             throw conversion_error{"Attempt to convert null to " + type + "."}; 
        
           }

libpqxx/include/pqxx/field.hxx

Lines 527 to 544 in 9d2a459

    
           /// Convert a field's value to type `T`. 
        
           /** Unlike the "regular" `from_string`, this knows how to deal with null 
        
            * values. 
        
            */ 
        
           template<typename T> inline T from_string(field const &value) 
        
           { 
        
             if (value.is_null()) 
        
             { 
        
               if constexpr (nullness<T>::has_null) 
        
                 return nullness<T>::null(); 
        
               else 
        
                 internal::throw_null_conversion(type_name<T>); 
        
             } 
        
             else 
        
             { 
        
               return from_string<T>(value.view()); 
        
             } 
        
           }

libpqxx/include/pqxx/strconv.hxx

Lines 402 to 420 in 9d2a459

    
           /// Macro: Define a string conversion for an enum type. 
        
           /** This specialises the @c pqxx::string_traits template, so use it in the 
        
            * @c ::pqxx namespace. 
        
            * 
        
            * For example: 
        
            * 
        
            *      #include <iostream> 
        
            *      #include <pqxx/strconv> 
        
            *      enum X { xa, xb }; 
        
            *      namespace pqxx { PQXX_DECLARE_ENUM_CONVERSION(x); } 
        
            *      int main() { std::cout << pqxx::to_string(xa) << std::endl; } 
        
            */ 
        
           #define PQXX_DECLARE_ENUM_CONVERSION(ENUM)                                    \ 
        
             template<> struct string_traits<ENUM> : pqxx::internal::enum_traits<ENUM>   \ 
        
             {};                                                                         \ 
        
             template<> inline std::string_view const type_name<ENUM>                    \ 
        
             {                                                                           \ 
        
               #ENUM                                                                     \ 
        
             }

This is the problem caused by 613f068.

I think we should change it to throw_null_conversion(std::string_view) or assign type_name<T> to temporary std::string variable in from_string(), but both functions are called in many places I can't determine how much this change would affect them, since they are both functions called in many places.

tt4g · 2024-03-19T04:05:04Z

Apparently explicit keyword was added to the std::basic_string<ChartT> constructor that receives std::basic_string_view<ChartT> in C++20.
The constructor in section 10 of cppreference: https://en.cppreference.com/w/cpp/string/basic_string/basic_string
This change results in no type conversion and the compile error.

alexolog · 2024-03-19T12:56:05Z

Yes, std::string_view is not implicitly convertible to std::string.
I believe it was like that since the beginning (C++17).

alexolog · 2024-03-19T21:02:56Z

Can you fix it?

tt4g · 2024-03-19T21:08:08Z

I would like to leave it to @jtv, as I cannot determine which of the following two methods would be better (less impact on the API) to fix.

Replace throw_null_conversion(std::string const &) with throw_null_conversion(std::string_view).
Assign the return value of type_name<T> to a temporary std::string variable in from_string(field const &)

jtv · 2024-03-19T21:31:01Z

Thanks @tt4g! I seem to have a nasty 'flu at the moment so I'm not at my best, and your analysis helps a ton.

First thought is perhaps we can make it so we can get a pqxx::zview instead of a std::string_view. (Bearing in mind that std::string is guaranteed to be null-terminated since C++14.)

But it's still strange to me that this does not happen in testing. I regularly build and test against C++17, C++20, and C++23! @alexolog, for completeness, could you show me a minimal reproduction of the problem? In other words, the absolute minimal stand-alone code that displays the problem?

I think maybe it's that I wrote tests for enum conversions before I had streaming queries. Streaming queries are the only case where we call throw_null_conversion() on a type that does not have a bult-in null.

jtv · 2024-03-19T21:35:05Z

I now suspect that I will need to split throw_null_conversion() into two functions as well, for conversions in the two directions.

alexolog · 2024-03-19T21:35:17Z

But it's still strange to me that this does not happen in testing. I regularly build and test against C++17, C++20, and C++23! @alexolog, for completeness, could you show me a minimal reproduction of the problem? In other words, the absolute minimal stand-alone code that displays the problem?

It will take a bit since we use a framework that wraps pqxx and I haven't used it directly in a while. I'll whip something up when I have a bit of spare time.

jtv · 2024-03-20T00:13:03Z

Thanks. Meanwhile yes, a switch to std::string_view does sound like a good quick fix.

There's a bit of a conundrum there with minimising string copies but let's be honest - exceptions are not what we optimise for. If we wanted it really nice we could create a constexpr template variable with the full exception string for any given type.

alexolog · 2024-04-08T18:00:56Z

Minimal sample:

#include "pqxx/pqxx"

enum class Enum { Zero = 0 };

namespace pqxx {
PQXX_DECLARE_ENUM_CONVERSION(Enum);
}

int main()
{
    pqxx::connection c;
    pqxx::work w{c};

    std::tuple<Enum> t;
    w.exec1("select 0").to(t);
}

Compilation Errors:

lib/pqxx/arm64-macOS/include/pqxx/field.hxx:538:39: error: no viable conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'const std::string' (aka 'const basic_string<char>')
  538 |       internal::throw_null_conversion(type_name<T>);
      |                                       ^~~~~~~~~~~~
lib/pqxx/arm64-macOS/include/pqxx/row.hxx:567:24: note: in instantiation of function template specialization 'pqxx::from_string<Enum>' requested here
  567 |   std::get<index>(t) = from_string<field_type>(f);
      |                        ^
lib/pqxx/arm64-macOS/include/pqxx/row.hxx:268:6: note: in instantiation of function template specialization 'pqxx::row::extract_value<std::tuple<Enum>, 0UL>' requested here
  268 |     (extract_value<Tuple, indexes>(t), ...);
      |      ^
lib/pqxx/arm64-macOS/include/pqxx/row.hxx:243:5: note: in instantiation of function template specialization 'pqxx::row::extract_fields<std::tuple<Enum>, 0UL>' requested here
  243 |     extract_fields(t, std::make_index_sequence<std::tuple_size_v<Tuple>>{});
      |     ^
lib/pqxx/arm64-macOS/include/pqxx/row.hxx:176:5: note: in instantiation of function template specialization 'pqxx::row::convert<std::tuple<Enum>>' requested here
  176 |     convert(t);
      |     ^
main.cpp:15:25: note: in instantiation of function template specialization 'pqxx::row::to<std::tuple<Enum>>' requested here
   15 |     w.exec1("select 0").to(t);
      |                         ^
/opt/homebrew/opt/llvm/include/c++/v1/string:896:33: note: candidate constructor not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'const string &' for 1st argument
  896 |   _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(const basic_string& __str)
      |                                 ^            ~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/homebrew/opt/llvm/include/c++/v1/string:913:55: note: candidate constructor not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'string &&' for 1st argument
  913 |   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(basic_string&& __str)
      |                                                       ^            ~~~~~~~~~~~~~~~~~~~~
/opt/homebrew/opt/llvm/include/c++/v1/string:937:55: note: candidate constructor template not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'const char *' for 1st argument
  937 |   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(const _CharT* __s)
      |                                                       ^            ~~~~~~~~~~~~~~~~~
/opt/homebrew/opt/llvm/include/c++/v1/string:951:3: note: candidate constructor not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'nullptr_t' (aka 'std::nullptr_t') for 1st argument
  951 |   basic_string(nullptr_t) = delete;
      |   ^            ~~~~~~~~~
/opt/homebrew/opt/llvm/include/c++/v1/string:1083:55: note: candidate constructor not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'initializer_list<char>' for 1st argument
 1083 |   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(initializer_list<_CharT> __il)
      |                                                       ^            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/homebrew/opt/llvm/include/c++/v1/string:886:64: note: explicit constructor is not a candidate
  886 |   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit basic_string(const allocator_type& __a)
      |                                                                ^
/opt/homebrew/opt/llvm/include/c++/v1/string:1039:93: note: explicit constructor is not a candidate
 1039 |   _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit basic_string(const _Tp& __t)
      |                                                                                             ^
lib/pqxx/arm64-macOS/include/pqxx/internal/conversions.hxx:58:42: note: passing argument to parameter 'type' here
   58 | throw_null_conversion(std::string const &type);
      |                                          ^
1 error generated.

jtv · 2024-04-23T19:31:04Z

Every time I get back into this issue, something distracts me. :-( Haven't forgotten about it, but various loved ones getting hit with various medical issues and such.

jtv · 2024-04-23T19:54:56Z

@alexolog, @tt4g — I finally made some time to focus on this and really, it seems pretty easy. I added a specialisation for throw_null_conversion(std::string_view). As far as I can see that's ABI-compatible.

And for a free bonus I used pqxx::internal::concat() to make it more efficient, because why not. (I'll probably use the standard formatting library in libpqxx 8, since that will require C++20.)

Fixes #807. Type names for user-defined enums are stored as `std::string_view`, not as `std::string` like the others. (Both are `inline` though.) The problem did not affect the conversion as such, and that is why a pure unit test was not enough to catch it. But it did show up when converting a `pqxx::field` value.

jtv · 2024-04-23T20:00:30Z

@alexolog perhaps you can try the proposed fix in #824?

alexolog · 2024-04-24T00:05:03Z

Can I just drop the header in or do I have to rebuild the library?

jtv · 2024-04-24T00:11:33Z

You'll need to rebuild the library. But if you still have your build directory, you could just drop the files I changed into your source tree, recompile, and reinstall. No need to rerun the configure step if you don't want to.

Fixes #807. Type names for user-defined enums are stored as `std::string_view`, not as `std::string` like the others. (Both are `inline` though.) The problem did not affect the conversion as such, and that is why a pure unit test was not enough to catch it. But it did show up when converting a `pqxx::field` value.

jtv mentioned this issue Apr 23, 2024

Fix compile error in field enum support. #824

Merged

jtv closed this as completed in #824 May 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PQXX_DECLARE_ENUM_CONVERSION issues #807

PQXX_DECLARE_ENUM_CONVERSION issues #807

alexolog commented Mar 19, 2024 •

edited

Loading

tt4g commented Mar 19, 2024

tt4g commented Mar 19, 2024

alexolog commented Mar 19, 2024 •

edited

Loading

alexolog commented Mar 19, 2024

tt4g commented Mar 19, 2024 •

edited

Loading

jtv commented Mar 19, 2024

jtv commented Mar 19, 2024

alexolog commented Mar 19, 2024

jtv commented Mar 20, 2024

alexolog commented Apr 8, 2024 •

edited

Loading

jtv commented Apr 23, 2024

jtv commented Apr 23, 2024

jtv commented Apr 23, 2024

alexolog commented Apr 24, 2024

jtv commented Apr 24, 2024

PQXX_DECLARE_ENUM_CONVERSION issues #807

PQXX_DECLARE_ENUM_CONVERSION issues #807

Comments

alexolog commented Mar 19, 2024 • edited Loading

tt4g commented Mar 19, 2024

tt4g commented Mar 19, 2024

alexolog commented Mar 19, 2024 • edited Loading

alexolog commented Mar 19, 2024

tt4g commented Mar 19, 2024 • edited Loading

jtv commented Mar 19, 2024

jtv commented Mar 19, 2024

alexolog commented Mar 19, 2024

jtv commented Mar 20, 2024

alexolog commented Apr 8, 2024 • edited Loading

jtv commented Apr 23, 2024

jtv commented Apr 23, 2024

jtv commented Apr 23, 2024

alexolog commented Apr 24, 2024

jtv commented Apr 24, 2024

alexolog commented Mar 19, 2024 •

edited

Loading

alexolog commented Mar 19, 2024 •

edited

Loading

tt4g commented Mar 19, 2024 •

edited

Loading

alexolog commented Apr 8, 2024 •

edited

Loading