Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std::char_traits<std::byte> removed in libc++ 18 #726

Closed
tambry opened this issue Aug 18, 2023 · 13 comments · Fixed by #751
Closed

std::char_traits<std::byte> removed in libc++ 18 #726

tambry opened this issue Aug 18, 2023 · 13 comments · Fixed by #751

Comments

@tambry
Copy link
Contributor

tambry commented Aug 18, 2023

libc++ version 18 breaks compiling libpqxx:

[66/115] Building CXX object test/CMakeFiles/runner.dir/test72.cxx.o
FAILED: test/CMakeFiles/runner.dir/test72.cxx.o
/usr/bin/clang++ -DPQXX_SHARED -I/usr/include/postgresql -I/home/raul.tambre/deb/libpqxx/build/include -I/home/raul.tambre/deb/libpqxx/include -g -std=c++17 -MD -MT test/CMakeFiles/runner.dir/test72.cxx.o -MF test/CMakeFiles/runner.dir/test72.cxx.o.d -o test/CMakeFiles/runner.dir/test72.cxx.o -c /home/raul.tambre/deb/libpqxx/test/test72.cxx
In file included from /home/raul.tambre/deb/libpqxx/test/test72.cxx:1:
In file included from /usr/bin/../include/c++/v1/iostream:43:
In file included from /usr/bin/../include/c++/v1/ios:222:
In file included from /usr/bin/../include/c++/v1/__locale:21:
In file included from /usr/bin/../include/c++/v1/mutex:192:
In file included from /usr/bin/../include/c++/v1/__condition_variable/condition_variable.h:17:
In file included from /usr/bin/../include/c++/v1/__mutex/unique_lock.h:17:
In file included from /usr/bin/../include/c++/v1/__system_error/system_error.h:14:
In file included from /usr/bin/../include/c++/v1/__system_error/error_category.h:15:
In file included from /usr/bin/../include/c++/v1/string:622:
/usr/bin/../include/c++/v1/string_view:294:45: error: implicit instantiation of undefined template 'std::char_traits<std::byte>'
  294 |     static_assert((is_same<_CharT, typename traits_type::char_type>::value),
      |                                             ^
/home/raul.tambre/deb/libpqxx/include/pqxx/util.hxx:303:35: note: in instantiation of template class 'std::basic_string_view<std::byte>' requested here
  303 | std::basic_string_view<std::byte> binary_cast(TYPE const &data)
      |                                   ^
/usr/bin/../include/c++/v1/__string/char_traits.h:42:8: note: template is declared here
   42 | struct char_traits;
      |        ^

Relevant changelog entry from LLVM 17:

The base template for std::char_traits has been marked as deprecated and will be removed in LLVM 18. If you are using std::char_traits with types other than char, wchar_t, char8_t, char16_t, char32_t or a custom character type for which you specialized std::char_traits, your code will stop working when we remove the base template. The Standard does not mandate that a base template is provided, and such a base template is bound to be incorrect for some types, which could currently cause unexpected behavior while going undetected.

@jtv
Copy link
Owner

jtv commented Aug 18, 2023

Oh dear. This looks like a huge problem. And I'm on vacation right now, which severely limits my ability to look into it.

@tambry
Copy link
Contributor Author

tambry commented Aug 18, 2023

Whipped up a quick patch to get things compiling based on libc++'s now-removed generic implementation, but it definitely isn't clean and fit for a PR. Should give you an idea of what will be required though.

LLVM 17 is due to be out in the second half of September. That should still only serve deprecation warnings though. So no need to hurry terribly, just the people truly living at head are broken currently. 🙂

0001-Use-a-custom-std-char_traits-byte.patch

@tambry tambry changed the title libc++ version 18 incompatibility std::char_traits<std::byte> removed in libc++ 18 Aug 18, 2023
@jtv
Copy link
Owner

jtv commented Aug 18, 2023

Thanks! Really appreciate the help while I'm without a computer.

In the longer term, the problem is that I opted for strings and views of std::byte as the canonical representations of binary data.

@jtv
Copy link
Owner

jtv commented Aug 28, 2023

Hi @tambry — I'm back from my vacation. My laptop was also having a problem so I didn't take it with me.

Really appreciate the patch. Of course it makes me a bit nervous: what if the compiler does choose to specialise std::char_traits<std::byte>? Could do a configure-time check, perhaps.

And we don't have a lot of alternatives, do we? It would seem sensible to replace std::basic_string<std::byte> with std::vector<std::byte> as the canonical type for binary data. But then what replaces std::basic_string_view<std::byte> until we have std::span in C++20? Your approach strikes me as the only way out until libpqxx 8. I'd really hate to do pointer/length pairs or a custom view/span wrapper.

@tambry
Copy link
Contributor Author

tambry commented Aug 28, 2023

Really appreciate the patch.

Please keep in mind that I threw the patch together very quickly just to get it compiling!

Of course it makes me a bit nervous: what if the compiler does choose to specialise std::char_traits<std::byte>? Could do a configure-time check, perhaps.

You can just always pass in your custom char_traits as my patch does. std::byte is quite simple and well-specified so I don't think a custom one would be a compatibility issue necessitating using a compiler's when available. But then I think all users will have to update code that uses the methods returning std::string<std::byte>/std::string_view<std::byte> to then use it with pqxx's custom char_traits so this would be an API and ABI break.

So to keep compatibility with existing code you'd probably want an alias to std::basic_string<byte> if std::char_traits<std::byte> exists, otherwise one with a custom char_traits. Users will need to update their code to use that custom alias/char_traits to get their code compiling again on libc++.

It would seem sensible to replace std::basic_stringstd::byte with std::vectorstd::byte as the canonical type for binary data.

Agreed.
Whatever can be currently done will feel like an interim solution until you can require C++20 and just use std::vector<std::byte>+std::span.

But then what replaces std::basic_string_view<std::byte> until we have std::span in C++20?

Nothing other than a custom wrapper, but that would be both an API and ABI breaking change.

@tambry
Copy link
Contributor Author

tambry commented Sep 18, 2023

This removal was reverted in llvm/llvm-project@cce062d and deferred to LLVM 19. So there's some more time to handle this before user complaints roll in.

@jtv
Copy link
Owner

jtv commented Sep 20, 2023

That's good news @tambry. I think I'll just go with your proposed patch.

@jtv
Copy link
Owner

jtv commented Sep 20, 2023

Come to think of it... Care to make it a PR? Easier to review, and also means you automatically get credit in the logs etc. :-)

@tambry
Copy link
Contributor Author

tambry commented Sep 27, 2023

I'll have a go at a MR. Might take a few weeks as I'm quite busy of late. 🙂

@jtv
Copy link
Owner

jtv commented Oct 20, 2023

Perhaps for now I should start promoting the use of std::vector<std::byte> instead of std::basic_string<std::byte>. The annoying thing there is that the logical counterpart tostd::basic_string_view<std::byte> would be std::span<std::byte>... but it's not available in C++17. :-(

@tambry
Copy link
Contributor Author

tambry commented Nov 21, 2023

Finally got around to this. PR #751.

Copy link

There has been no activity on this ticket. Consider closing it.

@jtv
Copy link
Owner

jtv commented Jan 22, 2024

Still very much hoping to get this on board for a 7.9 release!

@jtv jtv closed this as completed in #751 Feb 9, 2024
jtv pushed a commit that referenced this issue Feb 9, 2024
…yte> (#751)

The standard doesn't specify a generic implementation of std::char_traits, only specializations for certain types.
For a good reason: it's unlikely to be correct for all types that it'll compile with it.

All standard libraries today however do provide a generic implementation as an extension, though it's bound to be incorrect for some types.
libc++ has deprecated its in version 18 and will remove it in version 19 to eliminate hard to find correctness issues stemming from this.

Replace with type aliases that will use a custom char_traits when the standard library lacks such a generic implementation.
Note that we don't unconditionally use the custom char_traits variant as it's a source-breaking change requiring the users to update all usages of `std::string<std::byte>` to `pqxx::bytes` (i.e. `std::string<std::byte, pqxx::byte_char_traits>`). Ditto `std::string_view<std::byte>` and `pqxx::bytes_view`.
But for implementations lacking a generic implementation `std::string<std::byte>` and `std::string_view<std::byte>` wouldn't compile anyway so it's fine.

The aliases are named as `bytes` and `bytes_view` with the intetion of switching them to `std::vector<std::byte>` and `std::span<std::byte>` in a future major release that requires C++20.

Fixes: #726

By Raul Tambre <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants