Fix compiler warning (clang, cxx17)

Thalhammer / jwt-cpp

A header only library for creating and validating json web tokens in c++

https://thalhammer.github.io/jwt-cpp/

MIT License

856 stars 233 forks source link

Fix compiler warning (clang, cxx17) #290

Closed JonasBorchelt closed 1 year ago

JonasBorchelt commented 1 year ago

warnings:

'wstring_convert<std::codecvt_utf8<wchar_t, 1114111, 0>>' is deprecated 'wstring_convert<std::codecvt_utf8<wchar_t, 1114111, 0>>' has been explicitly marked deprecated here

prince-chrismc commented 1 year ago

For the linters there's a command you can run :) https://github.com/Thalhammer/jwt-cpp/actions/runs/4939213168/jobs/8838847109?pr=290 its just a copy paste from the logs

prince-chrismc commented 1 year ago

Found this gem doing some research on the issue 🤣

The replacement is praying for C++23 to have proper Unicode support

3 yr. ago !remindme 2023

https://isocpp.org/files/papers/D2139R3.html#3.21

Seems like there's no replacement but the tests are passing so 🤞

prince-chrismc commented 1 year ago

replace the old/deprecated wstring_convert with the even older mbrtowc C api

Would it make sense to drop the the C++11 ifdef? Or have a seperate >= C++17 section?

Thalhammer commented 1 year ago

replace the old/deprecated wstring_convert with the even older mbrtowc C api

Would it make sense to drop the the C++11 ifdef? Or have a seperate >= C++17 section?

If we go with this solution we can certainly drop the ifdef because the c-style functions should be available in all C++ versions.

JonasBorchelt commented 1 year ago

Hey guys,

sorry for being late. Thank you both for the hint regarding the patch command.

@Thalhammer

You raise a good point. While it is generally advisable to avoid deprecated APIs, sometimes there isn't a clear replacement. As of my knowledge, there is no new C++ API that directly replaces std::wstring_convert. The code provided uses the C API as a workaround (as you already stated), which can be of course less clean and intuitive than the C++ API. Once std::text becomes available, it would be the recommended way to convert between string encodings in C++.
Another good point regarding the reserve, I will add it.
cstring is needed for std::strerror(errno)

@prince-chrismc Good point, I will drop it.

Thank you both for your feedback!

Thalhammer commented 1 year ago

@laoshanxi Do you still need support for GCC 4.8 ? I cancelled the check because the 18.04 ubuntu image has been deprecated end of 2022 and apparently has been removed now because it can't find a runner anymore. If you still need it, could you check if those changes work in your setup ?

laoshanxi commented 1 year ago

@laoshanxi Do you still need support for GCC 4.8 ? I cancelled the check because the 18.04 ubuntu image has been deprecated end of 2022 and apparently has been removed now because it can't find a runner anymore. If you still need it, could you check if those changes work in your setup ?

Let me have a try.

laoshanxi commented 1 year ago

I tried build jwt-cpp in gcc 4.8 works, sorry I can not trigger whole app-mesh which used jwt-cpp due to other dependency can not pass this compiler.

gcc version 4.8.5 20150623 (Red Hat 4.8.5-44) (GCC)

For me, I am not using this compiler now, Thanks for asking.