Skip to content

Conversation

@Orphis
Copy link

@Orphis Orphis commented Oct 17, 2025

Boost 1.89.0 is required to have a compatible standard library. Older versionis would use std::unary_function which is not supported anymore in C++17.

Using the proper internal type name boost::uuids::detail::sha1::digest_type result in 2 different behaviors depending on the Boost version. The type can either be an array of "unsigned int[5]" (older Boost) or "char[20]" (newer Boost) which the serialization code needs to handle.

The serialization code is also moved away from using sprintf as modern compilers will emit lots of different warnings or errors.

Copy link
Contributor

@pysco68 pysco68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "SHA1 to hex string" bit could be simpler and less error prone.

Why not reinterpret cast hash to const unsigned char* given it's POD and walk through the LUT byte by byte instead of having the two disinct codepaths.

Also (but I think we can ignore this for the time being) SHA-1 digest bytes are big-endian - boost's digest_type uses the underlying host endianess so this would produce broken hashes there (also the previous code was broken in that regard tbh)

Boost 1.89.0 is required to have a compatible standard library.
Older versionis would use std::unary_function which is not supported anymore
in C++17.

Using the proper internal type name boost::uuids::detail::sha1::digest_type
result in 2 different behaviors depending on the Boost version.
The type can either be an array of "unsigned int[5]" (older Boost) or
"char[20]" (newer Boost) which the serialization code needs to handle.

The serialization code is also moved away from using sprintf as modern
compilers will emit lots of different warnings or errors.
@Orphis
Copy link
Author

Orphis commented Oct 17, 2025

The less error prone thing to do would be not to use the internals of boost::uuid to calculate a SHA1, but instead use cryptopp or OpenSSL. :)

Since the code path with unsigned int is in the old version of Boost and kept as a compatibility layer, we should mainly focus our attention on the first branch.
The second branch is equivalent code as the previous one without the unsafe sprintf usage.

Copy link
Contributor

@pysco68 pysco68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ before merging this please circle back there's more moving parts involved that might need to be considered

hash_buf[i * 2 + 7] = int_to_hex[(hash[i] ) & 0xF];
}
} else {
throw std::runtime_error("Unsupported boost::uuids::detail::sha1::digest_type size");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should set ec here not throw, throw is only from the overload without ec, this is following the same pricinple as how boost.asio design APIs

If we could do static_assert it would be better, but it seems not possible in our c++ version.

Copy link
Contributor

@daminetreg daminetreg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm opposing the merge of this until it is tested in nxxm-src.

Also I'm against the plan to integrate it with tipi CMakeList generator in cmake-re as this is not going in the actual direction of moving everything to a cmake-re we have that was already done for this library.

Yes it's an header only library, nevertheless it should be consumed with proper package configurations boundaries as in https://github.com/forexample/package-example/.

The plan for this is currently being written and I don't want to hack some upgrade of boost and stop using the CMakeLists here.

So either we switch fully to a newer boost or we don't.

Also regarding the code behaving differently on different boost versions it not necessarily something we want to maintain.

If we would I would not be going for the throw, which could be done with a static_assert as already mentioned on another discussion channel with @Orphis. Or what I would find much more readable checking for boost versions with the prepocessor. But ultimately we just want one codepath, relating to the version we use as basis.

It's less elegant in some ways, but much easier to maintain.

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 this pull request may close these issues.

3 participants