mumble-voip / libmumble

C++17 Mumble library.
Other
31 stars 13 forks source link

Switch to our own Base64 implementation #13

Open davidebeatrici opened 1 year ago

davidebeatrici commented 1 year ago

Written in modern C++, it strictly adheres to the Base64 specification.

This means that alternate alphabets and/or the lack of padding are rejected.

A benchmark would be nice to have, once we switch to a proper test framework.

Krzmbrzl commented 1 year ago

A benchmark would be nice to have, once we switch to a proper test framework.

-> https://github.com/google/benchmark I have used that one before and it is pretty amazing.

davidebeatrici commented 1 year ago
Windows build failure ```c++ Error: D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): error C2672: 'std::min': no matching overloaded function found C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(74): note: could be '_Ty std::min(std::initializer_list<_Elem>)' D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: '_Ty std::min(std::initializer_list<_Elem>)': expects 1 arguments - 2 provided C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(71): note: or '_Ty std::min(std::initializer_list<_Elem>,_Pr)' D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: '_Ty std::min(std::initializer_list<_Elem>,_Pr)': could not deduce template argument for 'std::initializer_list<_Elem>' from 'unsigned long' C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(64): note: or 'const _Ty &std::min(const _Ty &,const _Ty &) noexcept()' D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: 'const _Ty &std::min(const _Ty &,const _Ty &) noexcept()': template parameter '_Ty' is ambiguous D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: could be 'gsl::span::size_type' D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: or 'unsigned long' D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: 'const _Ty &std::min(const _Ty &,const _Ty &) noexcept()': could not deduce template argument for 'const _Ty &' from 'gsl::span::size_type' C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(54): note: or 'const _Ty &std::min(const _Ty &,const _Ty &,_Pr) noexcept()' D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: 'const _Ty &std::min(const _Ty &,const _Ty &,_Pr) noexcept()': expects 3 arguments - 2 provided Error: D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): error C2672: 'std::min': no matching overloaded function found C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(74): note: could be '_Ty std::min(std::initializer_list<_Elem>)' D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: '_Ty std::min(std::initializer_list<_Elem>)': expects 1 arguments - 2 provided C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(71): note: or '_Ty std::min(std::initializer_list<_Elem>,_Pr)' D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: '_Ty std::min(std::initializer_list<_Elem>,_Pr)': could not deduce template argument for 'std::initializer_list<_Elem>' from 'unsigned long' C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(64): note: or 'const _Ty &std::min(const _Ty &,const _Ty &) noexcept()' D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: 'const _Ty &std::min(const _Ty &,const _Ty &) noexcept()': template parameter '_Ty' is ambiguous D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: could be 'gsl::span::size_type' D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: or 'unsigned long' D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: 'const _Ty &std::min(const _Ty &,const _Ty &) noexcept()': could not deduce template argument for 'const _Ty &' from 'gsl::span::size_type' C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(54): note: or 'const _Ty &std::min(const _Ty &,const _Ty &,_Pr) noexcept()' D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: 'const _Ty &std::min(const _Ty &,const _Ty &,_Pr) noexcept()': expects 3 arguments - 2 provided ```

This is of course due to size_t being unsigned long long instead of unsigned long.

Unfortunately the proper literal was only introduced in C++23: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0330r8.html

We can get away with size_t{ value } though.

Krzmbrzl commented 1 year ago

Oh and another thing to consider: As you want to implement this header-only anyway, we could also template the functions to allow any type of the correct size (or anything that has at least the appropriate size). That way, we wouldn't have the uint8_t vs std::byte vs char vs ... problem

davidebeatrici commented 1 year ago

The failing Windows builds are probably the due to a missing <stdexcept> include.

Oh and another thing to consider: As you want to implement this header-only anyway, we could also template the functions to allow any type of the correct size (or anything that has at least the appropriate size). That way, we wouldn't have the uint8_t vs std::byte vs char vs ... problem

Or rather, template wrappers for them: thanks to the spans we can get away with simple casts.

Krzmbrzl commented 1 year ago

thanks to the spans we can get away with simple casts.

Doesn't seem like it: https://godbolt.org/z/5P5dWc5nj :eyes:

davidebeatrici commented 1 year ago

Sorry, by "simple" I meant casting the element type and specifying the size accordingly.

Krzmbrzl commented 1 year ago

Maybe we should define our own span_cast that does this under the hood. That will probably make our code a bit cleaner

davidebeatrici commented 1 year ago

Indeed, I was actually thinking about it. There are quite a few places in our codebase where it would be useful.

Krzmbrzl commented 1 year ago

Just to be explicit: The only thing missing from my POV is the documentation (+ the mentioned in-source explanatory comments)