From b4163877e722298b42f6e96831bd8ffef7785a20 Mon Sep 17 00:00:00 2001 From: Stephan Bergmann Date: Thu, 2 Jun 2022 15:14:56 +0200 Subject: Use more appropriate return type for ThreadPool::getPreferredConcurrency All call sites already effectively asked for an unsigned return type, including: * The ThreadPool ctor took an nWorkers argument of type sal_Int32, but internally stores that as std::size_t mnMaxWorkers. * ZipOutputStream::reduceScheduledThreadTasksToGivenNumberOrLess apparently benefits from an unsigned nThreadTasks parameter, getting rid of various casts in its implementation that were necessary to silence signed vs. unsigned comparison warnings. The only drawback is that comphelper::ThreadPool::getPreferredConcurrency() * 4 in package/source/zippackage/ZipPackageStream.cxx would now silently wrap around instead of causing UB on overflow (which could be detected with appropriate tools). Ideally, it would use some o3tl::saturating_mul if we had that, so add a TODO comment for now. While std::thread::hardware_concurrency returns unsigned, it looked more natural to go with std::size_t here, as some call sites already used that (see above), so the implementation of ThreadPool::getPreferredConcurrency is a natural place to hide clamping std::thread::hardware_concurrency() to std::size_t (in the unlikely case that std::size_t is of smaller rank than unsigned). This required addition of o3tl::clamp_to_unsigned in o3tl/safeint.hxx. Change-Id: I0a04a8b32e63ebfeb39f924c4b38520455a6fb38 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/135309 Tested-by: Jenkins Reviewed-by: Stephan Bergmann --- include/comphelper/threadpool.hxx | 5 +++-- include/o3tl/safeint.hxx | 10 ++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/include/comphelper/threadpool.hxx b/include/comphelper/threadpool.hxx index a3c3091ce483..84f9dc9284f6 100644 --- a/include/comphelper/threadpool.hxx +++ b/include/comphelper/threadpool.hxx @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -55,9 +56,9 @@ public: /// limit to avoid spawning an unnecessarily /// large number of threads on high-core boxes. /// MAX_CONCURRENCY env. var. controls the cap. - static sal_Int32 getPreferredConcurrency(); + static std::size_t getPreferredConcurrency(); - ThreadPool( sal_Int32 nWorkers ); + ThreadPool( std::size_t nWorkers ); ~ThreadPool(); /// push a new task onto the work queue diff --git a/include/o3tl/safeint.hxx b/include/o3tl/safeint.hxx index 85c61f8c33d3..027d283342b5 100644 --- a/include/o3tl/safeint.hxx +++ b/include/o3tl/safeint.hxx @@ -208,6 +208,16 @@ make_unsigned(T value) return value; } +template constexpr std::enable_if_t, T1> +clamp_to_unsigned(T2 value) { + if constexpr (std::is_unsigned_v) { + return value <= std::numeric_limits::max() ? value : std::numeric_limits::max(); + } else { + static_assert(std::is_signed_v); + return value < 0 ? 0 : clamp_to_unsigned(make_unsigned(value)); + } +} + // An implicit conversion from T2 to T1, useful in places where an explicit conversion from T2 to // T1 is needed (e.g., in list initialization, if the implicit conversion would be narrowing) but // tools like -fsanitize=implicit-conversion should still be able to detect truncation: -- cgit