Use a custom std::char_traits<byte> for implementations lacking a generic implementation #751

tambry · 2023-11-21T08:27:14Z

Not sure what style you'd prefer the docstrings in (byte_string or pqxx::byte_string).

We could also name these pqxx::bytes and pqxx::bytes_view to allow switching to std::vector<std::byte> and std::span<std::byte> in the next major version that bumps the C++ requirement without ending up with incorrect names for them.

Standard does not require a generic implementation. libc++ has deprecated its in version 18 and will remove it in version 19.

Replace with type aliases that will use a custom char_traits for libc++ 18 and newer. This is made conditional for libc++ 18 and newer only as this is a source breaking change.

Fixes: #726

jtv · 2023-11-22T01:16:43Z

Thank you for doing this! And perfect timing, too.

It may take me a few days to look this over again. I'll try not to bother you with petty details like how to spell the names in documentation — I can edit those myself later.

The names you suggest sound like a good idea, though I'll have to look around older libpqxx versions because I think at one point I had a different class for this called pqxx::bytes.

jtv · 2023-11-22T01:21:11Z

Oh, some build errors:

../include/pqxx/util.hxx:283:5: error: '_LIBCPP_VERSION' is not defined, evaluates to 0 [-Werror,-Wundef]

That's because I like to build libpqxx with the strictest compiler options I can, so that build problems will be more likely to show up in my build than other people's.

I suppose these lines:

#if _LIBCPP_VERSION >= 180000

...should look more like:

#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 180000

But perhaps checking the library version can be a little brittle as well? Is there a better condition?

tambry · 2023-11-22T09:56:11Z

It may take me a few days to look this over again. I'll try not to bother you with petty details like how to spell the names in documentation — I can edit those myself later.

I'll be happy to fix things up as you desire to hopefully not require any noisy follow-up commits on your part.

The names you suggest sound like a good idea, though I'll have to look around older libpqxx versions because I think at one point I had a different class for this called pqxx::bytes.

I've changed over the names to pqxx::bytes and pqxx::bytes_view.
I like that they're much shorter and enable the future migration to std::vector<std::byte> and std::span<std::byte> without ending up with illogical names, doing another rename or having multiple aliases for the same type.

But perhaps checking the library version can be a little brittle as well? Is there a better condition?

I didn't immediately see a way to do that, but thinking a little harder I've now managed to use SFINAE to make this generic and not specific to libc++. 🎉

All tests pass for me locally with PostgreSQL 16 with both a generic std::char_traits and it being absent.
I've also significantly improved the commit message. 😉

jtv · 2023-11-26T22:19:19Z

I'm sorry for being so slow — lots of drama.

Another small note: I actually have no idea what the rules are for multi-line strings in NEWS. So I keep them to one line per entry. But if you know better, that's fine with me. :-D

[UPDATE: I left out the "I" in "I keep them to one line per entry." Completely changed the meaning, sorry.]

include/pqxx/util.hxx

tambry · 2023-11-26T22:54:54Z

@jtv Basically the whole implementation of byte_char_traits is a functional-equivalent of libc++'s generic std::char_traits. I didn't think much about it when writing the original patch as I wanted to get it just compiling without any change in behaviour when I did the original patch locally. Will go over with actual thought now. 😅

include/pqxx/util.hxx

jtv · 2023-11-26T23:13:07Z

I've read through it now. Once again thanks for doing this! My review style dictates that I always have notes on a patch... Please don't be discouraged. :-)

jtv · 2023-11-26T23:14:47Z

Oh, you asked about docstring styles. Same as everywhere else...

Doxygen-style comments
with Markdown-style markup
on basically everything.

tambry · 2023-11-29T17:40:21Z

FYI I'm quite busy this week so I'll probably get around to this next Tuesday.
But I'll try sooner if I have some downtime!

jtv · 2023-11-30T02:44:29Z

Thanks for keeping me updated. Looking forward to this.

I've reserved significant time for libpqxx over December. Hopefully we can get a solid 7.9 ready for release, and lay more groundwork for 8.0 which will require C++20 as a minimum. These are also two opportunities to make incompatible changes.

jtv · 2023-12-05T03:15:51Z

@tambry I've got some other changes lined up, but when I think any would conflict with your work, I'll try to hold them until yours is merged.

jtv · 2023-12-12T00:22:41Z

@tambry just a gentle reminder so you don't forget. Or if you're at PGConf.eu, maybe we can just discuss it in person. :-)

jtv · 2023-12-22T14:58:02Z

I've got a few PRs waiting to be merged that should only conflict with this one on the NEWS file. Perhaps I should go ahead and merge those - it'll be annoying for you, for which I apologise, but not complicated

tambry · 2023-12-22T17:41:51Z

I'm very much familiar with rebasing and solving conflicts so won't be an issue. I should hopefully be able to finish this up over the holidays. Been really busy, sorry! 😅

jtv · 2023-12-23T00:52:36Z

Excellent, I'll merge a few then. Thanks for sticking with it, and... merry Christmas!

jtv · 2024-01-16T20:47:01Z

@tambry perhaps you can merge or rebase the latest upstream changes?

jtv · 2024-01-20T16:19:50Z

@tambry I took the liberty of resolving conflicts in the Github web UI. (Forgot you could do that.)

hope that doesn't conflict with any changes you may have waiting on your own system. I feel a bit stuck — I'd love to release a 7.9 soon with this change on board, as well as extensive fixes to improve compliance with the C++ Core Guidelines etc.

The latter I could merge first, but it may increase the conflict with any local changes that you may have pending.

jtv · 2024-01-22T23:20:25Z

My apologies @tambry — it looks like I left a double declaration of quote_raw() in there while I was trying to fix the conflicts.

tambry · 2024-01-22T23:25:14Z

@jtv Sorry for the lack of replies. My situation with not being caught up with both work and personal things remains the same, but I feel like I some hope of not getting more left behind. 😞
I'll try to get around to this tomorrow as this has been buzzing in my head for at least 1.5 months by now.

jtv · 2024-01-23T15:50:29Z

I'm sorry for nagging you about it... If you feel like you're not going to have the time, just let me know and I'll continue it based on your branch.

tambry · 2024-01-23T18:04:42Z

Rebased
Added comments
Added avoidance for libc++ 18's deprecation warning
Changed byte_char_traits to use std::mem*() and std::strlen() instead of implementing ourselves.

I've closed threads that I feel I dealt with sufficiently. Please do re-open in case of any questions or if you feel I resolved them inadequately.

…yte> 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

jtv · 2024-02-09T22:03:13Z

I guess it's my turn to apologise @tambry... You did lots of work two weeks ago and I hadn't noticed! Didn't see any notifications from githubfor some reason. And of course I was traveling for FOSDEM etc.

Thanks again for doing this work. I'm happy with the explanations and the changes, so I'd like to merge it. I'll make some minor tweaks such as applying the Doxygen comment format, and I'll try to make sure the documentation is up to date on the change.

@tambry

Patches up some very minor details after merging @tambry's great contribution in #751. After this I'll run the automatic formatter as well.

jtv · 2024-02-09T23:31:35Z

@tambry I think I have a solution for the length() problem: the function has to be declared but I can just not define it!

@tambry

* Some minor tweaks for `char_traits` work. Patches up some very minor details after merging @tambry's great contribution in #751. After this I'll run the automatic formatter as well. * Don't define char traits' `length()`. This function is just not appropriate to a `std::byte` specialisation. There is no way inspect the contents of a raw byte string and figure out from there what the string's length is. So, remove the function's definition. The _declaration_ is required, but there can't be a usable definition.

tambry · 2024-02-10T13:07:49Z

No worries, I'm still swamped, but at least we're through with this now. 🙂

@tambry I think I have a solution for the length() problem: the function has to be declared but I can just not define it!

Makes sense! You'll probably want to do the same for eof().

And of course I was traveling for FOSDEM etc.

Oh, I was at FOSDEM too. 😀
Pity we missed the opportunity to meet!

jtv · 2024-02-10T13:23:31Z

Absolutely! I tried wearing my PGDay badge but it broke.

tambry mentioned this pull request Nov 21, 2023

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

Closed