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

Set minimum supported version of Clang to 17 #4528

Closed
wants to merge 4 commits into from

Conversation

randombit
Copy link
Owner

@randombit randombit commented Jan 3, 2025

@randombit randombit added this to the Botan 3.8.0 milestone Jan 3, 2025
@randombit randombit force-pushed the jack/min-clang-is-18 branch from caa293f to 3ca1c65 Compare January 3, 2025 19:17
@coveralls
Copy link

coveralls commented Jan 3, 2025

Coverage Status

coverage: 91.285% (+0.02%) from 91.267%
when pulling 353dc8d on jack/min-clang-is-18
into 2c1135f on master.

@randombit randombit changed the title Set minimum supported version of Clang to 18 Set minimum supported version of Clang to 17 Jan 4, 2025
Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

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

All for it, but what about semver guarantees?

Comment on lines -109 to -124
// TODO: C++20 introduces std::source_location which will allow to eliminate this
// macro altogether. Instead, using code would just call the C++ function
// that makes use of std::source_location like so:
//
// template<typename T, uint32_t M, typename F>
// int botan_ffi_visit(botan_struct<T, M>* obj, F func,
// const std::source_location sl = std::source_location::current())
// {
// // [...]
// if constexpr(...)
// {
// return ffi_guard_thunk(sl.function_name(), [&] { return func(*p); })
// }
// // [...]
// }
#define BOTAN_FFI_VISIT(obj, lambda) botan_ffi_visit(obj, lambda, __func__)
Copy link
Collaborator

Choose a reason for hiding this comment

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

🥳

@randombit
Copy link
Owner Author

All for it, but what about semver guarantees?

Personally I view SemVer and the minimum supported compiler version as distinct issues. Admittedly this is not spelled out so concretely, only alluded to in statements in the docs like

we fix the minimum required compiler version and aim to maintain that support

In retrospect specifying the minimum Clang version in 3.0.0 rather than floating it like XCode/NDK was a mistake, since it was known at the time that Clang was well behind on C++20 features. Alas.

I can't find many comparable situations wrt required toolchain updates. C is pretty stagnant so it doesn't come up much. In Rust you just bump the minor version. Apps like Chrome do this (I guess they just recently started requiring Clang 16) but it's not quite the same situation as for a library that has direct downstream consumers.

For context main motivation here is that with a bit of work we can capture the location of all exception throws with std::source_location which would IMO be a quite useful debugging facility for end users. The typename stuff/etc is an annoyance but not a blocker.

Maybe not worth it though.

@reneme
Copy link
Collaborator

reneme commented Jan 6, 2025

it's not quite the same situation as for a library that has direct downstream consumers.

That's exactly my concern.

With a bit of work we can capture the location of all exception throws with std::source_location which would IMO be a quite useful debugging facility for end users.

I believe std::source_location also "works" (read: compiles) in clang 14, it just returns the wrong results. I may be wrong, though. Nevertheless: for this particular use case we might be able to get away with some compiler-specific special cases, no?

Maybe not worth it though.

Newer compiler is always worth it, in my personal opinion. Though, I feel we did upset some users with the jump to C++20 (most notable the automobile industry that is bound to MISRA, and thus C++17).

@randombit
Copy link
Owner Author

Though, I feel we did upset some users with the jump to C++20 (most notable the automobile industry that is bound to MISRA, and thus C++17).

I love how MISRA, focused on avoiding runtime errors in the code, decided to standardize on the version of C++ that doesn't have span. 😭 IMO the reason for us to use C++20 instead of any prior version was span. Concepts are nice, ranges seem interesting to the extent we can use them at all, constinit/consteval are useful, but we could drop all of them and go back to C++17 and be fine, and I'd even consider it to give MISRA users an easier time. But no span? Nope, not happening.

You do have a point. I've been thinking for Botan4 if it makes sense to drop deprecated features and possibly bump compiler versions (eg GCC 13/Clang 17 so we finally have std::source_location and etc and can drop various compiler bug workarounds) but not increase the required version of C++. C++23 and from what I've seen C++26 don't have that much to offer that's specific to what's happening here. Some nice features that I'm sure we could make use of but maybe not worth the disruption it seems bumping language versions causes.. give the various embedded toolchains, MISRA-like standards etc a few years to catch up.

@reneme
Copy link
Collaborator

reneme commented Jan 11, 2025

Full ack on the above.

@Ashishbsharma has a point. Let's explore making the newer compilers a soft dependency, hide a compiler switch behind the existing macros, and start using source_location where we can.

I'm not sure it would buy us much benefit aside of getting some experience with the new features though, admittedly.

@randombit
Copy link
Owner Author

hide a compiler switch behind the existing macros, and start using source_location where we can.

Honestly it seems like it would be simpler at that point to just continue using the traditional __func__, __FILE__, __LINE__ plus a macro as we have been doing. Rather than a bunch of #if BOTAN_HAS_SOURCE_LOCATION etc etc and then two implementations to handle the different approaches.

At this point my thinking is maybe I should revert #4530, close this and just accept we're stuck with Clang 14 for the duration.

This conversation does get me thinking about the timing of Botan4. The previous logic of ~~ 2027 was based around the pipeline

  • GCC probably will mostly finish C++26 support in time for GCC 16 in spring 2026
  • Ubuntu 26.04 will probably ship with GCC 16, at least as an option if not the default
  • Github will take their sweet ass time providing an actions image but probably we'd have it by late 2026
  • Ergo we should viably ship a C++26 library with working CI in 2027

None of this is relevant if we just stick with C++20 for Botan4, the compilers in 24.04 are more than enough. So we could do something more like early 2026? IDK

@randombit
Copy link
Owner Author

Closing as wont-do. We'll just wait for Botan4 and at time time fix new minimum compiler versions

@randombit randombit closed this Jan 22, 2025
@randombit randombit deleted the jack/min-clang-is-18 branch January 22, 2025 21:21
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