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

fmt/chrono.h: use of deprecated std::codecvt class template which is deprecated as of C++20 #2408

Closed
JoeLoser opened this issue Jul 1, 2021 · 7 comments

Comments

@JoeLoser
Copy link

JoeLoser commented Jul 1, 2021

As of C++20, the class template speciailization std::codecvt<char32_t, char, std::mbstate_t> is deprecated in favor of using char8_t: std::codecvt<char32_t, char8_t, std::mbstate_t>.

The line causing this warning is chrono.h. This warning was noticed when using llvm12 with libc++ in C++20 mode (c++2a) as libc++ marks the class template as deprecated per the C++20 spec.

Some notes regarding a potential patch for this:

  • The do_write function currently returns a std::string. One approach to solving this problem is to change this do_write function to work with char or char8_t consistently based on compiler support for char8_t. This would mean that the return type is now std::string or std::basic_string<char8_t>. Consider the following snippet:
#if defined(__cpp_char8_t)
  using char_t = char8_t;
#else
  using char_t = char;
#endif
  auto&& os = std::basic_ostringstream<char_t>();

// Other code as-is

auto& f =
      std::use_facet<std::codecvt<code_unit, char_t, std::mbstate_t>>(loc);
  • The call to std::time_put notably should use char still rather than char_t. There is no overload for std::time_put<char8_t> which is an oversight I think when char8_t got proposed. It can be added in a future paper which I may propose.

Note: I recommend working with char_t as described above throughout to avoid UB. While char and char8_t are the same size, alignment, etc, it is UB to reinterpret_cast different types (e.g. a std::string to a std::basic_string<char8_t>) for example as returned from the stringstream.str() call.

With the return type of do_write being a std::string or std::basic_string<char8_t> now, I don't yet know how this affects the calling code copies the string to the output iterator.

@vitaut
Copy link
Contributor

vitaut commented Jul 1, 2021

A PR to suppress the warning is welcome. char8_t is not well-supported in the library and should generally be avoided.

@JoeLoser
Copy link
Author

JoeLoser commented Jul 1, 2021

A PR to suppress the warning is welcome. char8_t is not well-supported in the library and should generally be avoided.

While char8_t may not be well-supported, I'm not sure fmt should rely on deprecated standard library facilities, even though it will be quite some time (or possibly never) that the class template specialization is actually removed in libc++ for example.

With that being said, would you prefer just a local pragma push/pop of -Wdeprecated-declaration if char8_t feature test macro is true? I'm open to other suggestions and happy to make a PR in the direction we want to go. I didn't want to go down too far down the char8_t all the way through do_write and its calling code without more discussion here with you.

@vitaut
Copy link
Contributor

vitaut commented Jul 1, 2021

A local pragma would be fine.

Using deprecated API is OK here because it's a legacy path anyway and even if it is removed (which is unlikely) there is a reasonable fallback. Besides char8_t is a completely wrong thing here.

@vitaut
Copy link
Contributor

vitaut commented Aug 8, 2021

Suppressed the warning in d57b2a6. Thanks for reporting.

@vitaut vitaut closed this as completed Aug 8, 2021
@ivan-volnov
Copy link
Contributor

This hasn't been fixed.

include/fmt/chrono.h:324:24: warning: 'codecvt<char32_t, char, __mbstate_t>' is deprecated [-Wdeprecated-declarations]
  using codecvt = std::codecvt<CodeUnit, char, std::mbstate_t>;
                       ^

@vitaut
Copy link
Contributor

vitaut commented Jan 19, 2022

I guess we could replace the two uses of the codecvt alias with std::codecvt<CodeUnit, char, std::mbstate_t>. A PR would be welcome.

ivan-volnov added a commit to ivan-volnov/fmt that referenced this issue Jan 19, 2022
@ivan-volnov
Copy link
Contributor

Ok, done #2725

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

No branches or pull requests

3 participants