diff --git a/.clang-format b/.clang-format index 8f6cc725..70d30051 100644 --- a/.clang-format +++ b/.clang-format @@ -7,7 +7,7 @@ AccessModifierOffset: -4 ColumnLimit: 100 Language: Cpp -Standard: c++17 +Standard: c++20 AlignAfterOpenBracket: Align AlignEscapedNewlines: Right AllowAllArgumentsOnNextLine: false diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b05d714b..bfbcfb60 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -52,6 +52,12 @@ env: OPTREE_CXX_WERROR: "OFF" _GLIBCXX_USE_CXX11_ABI: "1" _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR: "1" + # Apple's libc++ gates `std::format` behind macOS 13.3 / iOS 16.3. iOS wheel tags carry the + # minor, so 16.3 is taggable exactly. macOS tags do not: PEP 425 floors the minor to zero above + # macOS 11, so a 13.3 binary would ship as `macosx_13_0_*`, install on 13.0-13.2 and fail in + # `dyld`. 14.0 is the lowest target whose tag does not lie. + MACOSX_DEPLOYMENT_TARGET: "14.0" + IPHONEOS_DEPLOYMENT_TARGET: "16.3" PYTHONUNBUFFERED: "1" PYTHON_TAG: "py3" # to be updated PYTHON_VERSION: "3" # to be updated diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 0a3c058b..e6a93690 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -102,7 +102,7 @@ jobs: run: make clang-format - name: clang-tidy - run: make clang-tidy CMAKE_CXX_STANDARD=17 + run: make clang-tidy CMAKE_CXX_STANDARD=20 - name: cpplint run: make cpplint diff --git a/.github/workflows/tests-with-pydebug.yml b/.github/workflows/tests-with-pydebug.yml index 9b72ae55..7a695040 100644 --- a/.github/workflows/tests-with-pydebug.yml +++ b/.github/workflows/tests-with-pydebug.yml @@ -46,6 +46,9 @@ env: OPTREE_CXX_WERROR: "ON" _GLIBCXX_USE_CXX11_ABI: "1" _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR: "1" + # Apple's libc++ gates `std::format` behind macOS 13.3; 14.0 keeps the wheel tag honest, see + # `build.yml`. + MACOSX_DEPLOYMENT_TARGET: "14.0" PYTHONDEVMODE: "1" PYTHONUNBUFFERED: "1" PYTHON: "python" # to be updated diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index e431cd65..49498328 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -44,6 +44,9 @@ env: OPTREE_CXX_WERROR: "ON" _GLIBCXX_USE_CXX11_ABI: "1" _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR: "1" + # Apple's libc++ gates `std::format` behind macOS 13.3; 14.0 keeps the wheel tag honest, see + # `build.yml`. + MACOSX_DEPLOYMENT_TARGET: "14.0" FULL_TEST_PYTHON_VERSIONS: "3.12;3.13" PYTHONDEVMODE: "1" PYTHONUNBUFFERED: "1" @@ -207,37 +210,6 @@ jobs: cat pyproject.toml echo "::endgroup::" - - name: Test installable with C++17 - shell: bash - if: runner.os != 'Windows' - run: | - ( - set -ex - ${{ env.PYTHON }} -m venv venv - source venv/bin/activate - OPTREE_CXX_WERROR=OFF CMAKE_CXX_STANDARD=17 \ - ${{ env.PYTHON }} -m pip install -v . - pushd tests - ${{ env.PYTHON }} -X dev -Walways -Werror -c 'import optree' - popd - rm -rf venv - ) - - if [[ "$?" -ne 0 ]]; then - echo "::error::Failed to install with C++17." >&2 - exit 1 - fi - CORE_DUMP_FILES="$( - find . -type d -path "./venv" -prune \ - -o '(' -iname "core.*.[1-9]*" -o -iname "core_*.dmp" ')' -print - )" - if [[ -n "${CORE_DUMP_FILES}" ]]; then - echo "::error::Coredump files found, indicating a crash during tests." >&2 - echo "Coredump files:" >&2 - ls -alh ${CORE_DUMP_FILES} >&2 - exit 1 - fi - - name: Test buildable without Python frontend if: runner.os != 'Windows' run: | diff --git a/CHANGELOG.md b/CHANGELOG.md index 45553613..b79e11f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Update minimal version of `typing-extensions` to 4.10.0 for `typing_extensions.TypeIs` by [@XuehaiPan](https://github.com/XuehaiPan) in [#285](https://github.com/metaopt/optree/pull/285). +- Raise the minimum required C++ standard for building the C extension from C++17 to C++20, for `std::format`, `std::source_location`, `std::span`, `std::ranges`, and constrained (`requires`) templates by [@XuehaiPan](https://github.com/XuehaiPan) in [#235](https://github.com/metaopt/optree/pull/235). Building from source now needs GCC 13+, Clang 16+ with libstdc++ 13+, Clang 17+ with libc++ 17+, Apple Clang 16+ (Xcode 16+), or MSVC 19.32+ (Visual Studio 2022 17.2+); `std::format` sets the floor on GCC and MSVC, and `P0634R3` sets it on Clang. +- Raise the deployment targets of the macOS and iOS wheels to 14.0 and 16.3, as Apple's libc++ gates `std::format` behind macOS 13.3 / iOS 16.3; macOS wheel tags floor the minor version to zero, so 14.0 is the lowest target that does not advertise support for 13.0-13.2, while the iOS wheels keep the exact 16.3 floor and are retagged to match, since cibuildwheel's cross-build environment would otherwise tag them `ios_13_0_*` by [@XuehaiPan](https://github.com/XuehaiPan) in [#235](https://github.com/metaopt/optree/pull/235). +- Stop passing `-Wno-error=attributes` under `OPTREE_CXX_WERROR=ON`; it existed so that the C++20 `[[likely]]`/`[[unlikely]]` attributes would not fail a C++17 build, and now only masks genuine attribute errors by [@XuehaiPan](https://github.com/XuehaiPan) in [#235](https://github.com/metaopt/optree/pull/235). ### Fixed @@ -68,7 +71,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Removed -- +- Remove support for building the C extension with C++17; `CMAKE_CXX_STANDARD` must now be at least `20` by [@XuehaiPan](https://github.com/XuehaiPan) in [#235](https://github.com/metaopt/optree/pull/235). ------ diff --git a/CMakeLists.txt b/CMakeLists.txt index e07a5eae..aa9c74d5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -37,9 +37,9 @@ if(NOT CMAKE_BUILD_TYPE) endif() message(STATUS "Build type: ${CMAKE_BUILD_TYPE}") -setdefault_ifndef(CMAKE_CXX_STANDARD 20) # for likely/unlikely attributes -if(CMAKE_CXX_STANDARD VERSION_LESS 17) - message(FATAL_ERROR "C++17 or higher is required") +setdefault_ifndef(CMAKE_CXX_STANDARD 20) +if(CMAKE_CXX_STANDARD VERSION_LESS 20) + message(FATAL_ERROR "C++20 or higher is required") endif() set(CMAKE_CXX_STANDARD_REQUIRED ON) message(STATUS "Use C++ standard: C++${CMAKE_CXX_STANDARD}") @@ -138,7 +138,9 @@ if(OPTREE_CXX_WERROR) if(MSVC) string(APPEND CMAKE_CXX_FLAGS " /WX") else() - string(APPEND CMAKE_CXX_FLAGS " -Werror -Wno-error=attributes -Wno-error=redundant-move") + # `-Wno-error=redundant-move` stays: C++20's implicit-move rules (P1825) change what + # `-Wredundant-move` fires on, and the warning differs between GCC and Clang. + string(APPEND CMAKE_CXX_FLAGS " -Werror -Wno-error=redundant-move") endif() endif() diff --git a/README.md b/README.md index 032b6754..dbae4acf 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ The following options are available while building the Python C extension from s ```bash export CMAKE_COMMAND="/path/to/custom/cmake" export CMAKE_BUILD_TYPE="Debug" -export CMAKE_CXX_STANDARD="20" # C++17 is tested on Linux/macOS (C++20 is required on Windows) +export CMAKE_CXX_STANDARD="20" # 20 is the minimum supported standard export OPTREE_CXX_WERROR="OFF" export _GLIBCXX_USE_CXX11_ABI="1" # set to 0 to use the old libstdc++ ABI export _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR="1" # set to "" to disable the workaround for MSVC mutex layout change in VS 2022 v17.10+ @@ -72,7 +72,7 @@ export pybind11_DIR="/path/to/custom/pybind11" pip3 install . ``` -Compiling from source requires Python 3.9+, a C++ compiler (`g++` / `clang++` / `icpx` / `cl.exe`) that supports C++20, and a `cmake` installation. +Compiling from source requires Python 3.9+, a `cmake` installation (3.18+), and a C++ compiler (`g++` / `clang++` / `icpx` / `cl.exe`) with complete C++20 support: GCC 13+, Clang 16+ with libstdc++ 13+, Clang 17+ with libc++ 17+, Apple Clang 16+ (Xcode 16+), or MSVC 19.32+ (Visual Studio 2022 17.2+). Two features set those floors: `std::format` on GCC and MSVC, and `P0634R3` (`typename` made optional) on Clang. On Apple platforms `std::format` is additionally gated behind macOS 13.3 / iOS 16.3 availability, so a build targeting an older release will not compile. -------------------------------------------------------------------------------- diff --git a/include/optree/exceptions.h b/include/optree/exceptions.h index ab1d1701..7a97d0db 100644 --- a/include/optree/exceptions.h +++ b/include/optree/exceptions.h @@ -17,65 +17,61 @@ limitations under the License. #pragma once -#include // std::size_t -#include // std::optional, std::nullopt -#include // std::ostringstream -#include // std::logic_error -#include // std::string, std::char_traits, std::to_string -#include // std::void_t, std::{true,false}_type -#include // std::declval +#include // std::size_t +#include // std::format +#include // std::source_location +#include // std::logic_error +#include // std::string, std::char_traits, std::to_string +#include // std::string_view namespace optree { -constexpr std::size_t CURRENT_FILE_PATH_SIZE = std::char_traits::length(__FILE__); +constexpr std::size_t CURRENT_FILE_PATH_SIZE = + std::char_traits::length(std::source_location::current().file_name()); constexpr std::size_t CURRENT_FILE_RELPATH_FROM_PROJECT_ROOT_SIZE = std::char_traits::length("include/optree/exceptions.h"); static_assert(CURRENT_FILE_PATH_SIZE >= CURRENT_FILE_RELPATH_FROM_PROJECT_ROOT_SIZE, "SOURCE_PATH_PREFIX_SIZE must be greater than 0."); constexpr std::size_t SOURCE_PATH_PREFIX_SIZE = CURRENT_FILE_PATH_SIZE - CURRENT_FILE_RELPATH_FROM_PROJECT_ROOT_SIZE; -// NOLINTNEXTLINE[bugprone-reserved-identifier] -#define __FILE_RELPATH_FROM_PROJECT_ROOT__ ((const char *)&(__FILE__[SOURCE_PATH_PREFIX_SIZE])) + +// Strip the prefix `SOURCE_PATH_PREFIX_SIZE` measured off this header's own path. A translation +// unit compiled with a shorter path is returned as-is rather than letting `substr` throw. +constexpr std::string_view RelpathFromProjectRoot(const std::string_view &abspath) { + return abspath.size() >= SOURCE_PATH_PREFIX_SIZE ? abspath.substr(SOURCE_PATH_PREFIX_SIZE) + : abspath; +} +constexpr std::string_view RelpathFromProjectRoot( + const std::source_location &source_location = std::source_location::current()) { + return RelpathFromProjectRoot(source_location.file_name()); +} class InternalError : public std::logic_error { public: - explicit InternalError(const std::string &message) noexcept(noexcept(std::logic_error{message})) - : std::logic_error{message} {} - explicit InternalError(const std::string &message, - const std::string &file, - const std::size_t &lineno, - const std::optional function = - std::nullopt) noexcept(noexcept(std::logic_error{message})) - : InternalError([&message, &file, &lineno, &function]() -> std::string { - std::ostringstream oss{}; - oss << message << " ("; - if (function) [[likely]] { - oss << "function `" << *function << "` "; - } - oss << "at file " << file << ":" << lineno << ")\n\n" - << "Please file a bug report at https://github.com/metaopt/optree/issues."; - return oss.str(); - }()) {} + explicit InternalError( + const std::string_view &message, + const std::source_location &source_location = std::source_location::current()) + : std::logic_error{ + std::format("{} (in function `{}` at file {}:{}:{})\n\n" + "Please file a bug report at https://github.com/metaopt/optree/issues.", + message, + source_location.function_name(), + RelpathFromProjectRoot(source_location), + source_location.line(), + source_location.column())} {} }; } // namespace optree inline namespace { // NOLINT[build/namespaces_headers] -// SFINAE helper to detect if std::to_string is available for a type -template -struct has_to_string : std::false_type {}; - +// Detect whether `std::to_string` accepts a `const T &`, which is how `try_to_string` calls it. template -struct has_to_string()))>> : std::true_type { -}; - -template -inline constexpr bool has_to_string_v = has_to_string::value; +concept has_to_string = requires(const T &value) { std::to_string(value); }; // Convert value to string if possible, otherwise return a placeholder. template inline std::string try_to_string([[maybe_unused]] const T &value) { - if constexpr (has_to_string_v) { + if constexpr (has_to_string) { return std::to_string(value); } return ""; @@ -85,16 +81,8 @@ inline std::string try_to_string([[maybe_unused]] const T &value) { #define VA_FUNC2_(__0, __1, NAME, ...) NAME #define VA_FUNC3_(__0, __1, __2, NAME, ...) NAME -#if !defined(__GNUC__) -# define __PRETTY_FUNCTION__ std::nullopt // NOLINT[bugprone-reserved-identifier] -#endif - #define INTERNAL_ERROR0_() INTERNAL_ERROR1_("Unreachable code.") -#define INTERNAL_ERROR1_(message) \ - throw optree::InternalError((message), \ - __FILE_RELPATH_FROM_PROJECT_ROOT__, \ - __LINE__, \ - __PRETTY_FUNCTION__) +#define INTERNAL_ERROR1_(message) throw optree::InternalError(message) #define INTERNAL_ERROR(...) \ VA_FUNC2_(__0 __VA_OPT__(, ) __VA_ARGS__, INTERNAL_ERROR1_, INTERNAL_ERROR0_)(__VA_ARGS__) diff --git a/include/optree/hashing.h b/include/optree/hashing.h index 405b4164..01e0ae3a 100644 --- a/include/optree/hashing.h +++ b/include/optree/hashing.h @@ -17,10 +17,11 @@ limitations under the License. #pragma once -#include // std::size_t -#include // std::hash, std::{not_,}equal_to -#include // std::string -#include // std::pair +#include // std::size_t +#include // std::hash, std::{not_,}equal_to +#include // std::string +#include // std::string_view +#include // std::pair #include @@ -123,3 +124,61 @@ struct std::hash> { } }; // NOLINTEND[bugprone-std-namespace-modification] + +namespace optree { + +// Transparent hashers and comparators for the registry's pair keys. Their `operator()` MUST be +// templates: `is_transparent` only tells the container it may forward a foreign key type, and a +// non-template call operator then converts it back to the exact `key_type` — the very temporary +// heterogeneous lookup exists to avoid. The marker was inert for unordered containers before +// P0919R3, so this began costing a namespace-string copy per `Lookup` only at C++20. +// `std::hash` is guaranteed to agree with `std::hash`, so probing +// with a view finds entries inserted with a string. + +// Key of `PyTreeTypeRegistry::m_named_registrations`: (namespace, type). +struct NamespacedTypeHash { + using is_transparent = void; + template + inline Py_ALWAYS_INLINE std::size_t operator()( + const std::pair &key) const noexcept { + std::size_t seed = 0; + HashCombine(seed, std::string_view{key.first}); + HashCombine(seed, key.second); + return seed; + } +}; +struct NamespacedTypeEqual { + using is_transparent = void; + template + inline Py_ALWAYS_INLINE bool operator()(const std::pair &lhs, + const std::pair &rhs) const noexcept { + // Compare the type first: it is a pointer identity test, and it discriminates far more + // often than the namespace does. + return lhs.second.is(rhs.second) && + std::string_view{lhs.first} == std::string_view{rhs.first}; + } +}; + +// Key of `PyTreeTypeRegistry::sm_dict_insertion_ordered_namespaces`: (interpreter, namespace). +struct InterpreterNamespaceHash { + using is_transparent = void; + template + inline Py_ALWAYS_INLINE std::size_t operator()( + const std::pair &key) const noexcept { + std::size_t seed = 0; + HashCombine(seed, key.first); + HashCombine(seed, std::string_view{key.second}); + return seed; + } +}; +struct InterpreterNamespaceEqual { + using is_transparent = void; + template + inline Py_ALWAYS_INLINE bool operator()(const std::pair &lhs, + const std::pair &rhs) const noexcept { + return lhs.first == rhs.first && + std::string_view{lhs.second} == std::string_view{rhs.second}; + } +}; + +} // namespace optree diff --git a/include/optree/pytypes.h b/include/optree/pytypes.h index 3ef2483b..98b4362b 100644 --- a/include/optree/pytypes.h +++ b/include/optree/pytypes.h @@ -19,9 +19,10 @@ limitations under the License. #include // std::size_t, offsetof #include // std::rethrow_exception, std::current_exception +#include // std::format, std::format_to, std::formatter #include // std::optional #include // std::string -#include // std::enable_if_t, std::is_same_v, std::is_base_of_v, std::conditional_t +#include // std::is_same_v, std::is_base_of_v, std::conditional_t #include // std::unordered_map #include // std::forward, std::pair, std::make_pair, std::move #include // std::vector @@ -54,6 +55,36 @@ namespace py = pybind11; return static_cast(py::repr(py::str(string))); } +// Format a Python object with `std::format` as its `repr()`, so error messages can interpolate `{}` +// instead of splicing `PyRepr()` calls. Pass a concrete wrapper (`py::list`, `py::type`, ...) as +// `py::handle{value}`; a `std::string` formats as itself, so keep `PyRepr()` for those. +// +// Do not collapse these into one specialization constrained on `std::is_base_of_v`: +// that shares the standard range formatter's argument list, and since neither constraint subsumes +// the other, every pybind11 wrapper (all input ranges) becomes ambiguous at C++23 and later. +template +struct std::formatter { + template + constexpr ParseContext::iterator parse(const ParseContext &context) { + return context.begin(); + } + template + FormatContext::iterator format(const py::handle &object, FormatContext &context) const { + return std::format_to(context.out(), "{}", PyRepr(object)); + } +}; +template +struct std::formatter { + template + constexpr ParseContext::iterator parse(const ParseContext &context) { + return context.begin(); + } + template + FormatContext::iterator format(const py::object &object, FormatContext &context) const { + return std::format_to(context.out(), "{}", PyRepr(object)); + } +}; + #define PyNoneTypeObject \ (py::reinterpret_borrow(reinterpret_cast(Py_TYPE(Py_None)))) #define PyTupleTypeObject \ @@ -107,18 +138,22 @@ namespace py = pybind11; #endif } -template >> +template [[nodiscard]] inline Py_ALWAYS_INLINE T TupleGetItemAs(const py::handle &tuple, - const py::ssize_t &index) { + const py::ssize_t &index) + requires(std::is_base_of_v) +{ return py::reinterpret_borrow(PyTuple_GET_ITEM(tuple.ptr(), index)); } [[nodiscard]] inline Py_ALWAYS_INLINE py::object TupleGetItem(const py::handle &tuple, const py::ssize_t &index) { return TupleGetItemAs(tuple, index); } -template >> +template [[nodiscard]] inline Py_ALWAYS_INLINE T ListGetItemAs(const py::handle &list, - const py::ssize_t &index) { + const py::ssize_t &index) + requires(std::is_base_of_v) +{ #if PY_VERSION_HEX >= 0x030D00A4 // Python 3.13.0a4 PyObject * const item = PyList_GetItemRef(list.ptr(), index); if (item == nullptr) [[unlikely]] { @@ -139,9 +174,10 @@ template (list, index); } -template >> -[[nodiscard]] inline Py_ALWAYS_INLINE T DictGetItemAs(const py::handle &dict, - const py::handle &key) { +template +[[nodiscard]] inline Py_ALWAYS_INLINE T DictGetItemAs(const py::handle &dict, const py::handle &key) + requires(std::is_base_of_v) +{ #if PY_VERSION_HEX >= 0x030D00A1 // Python 3.13.0a1 PyObject *value = nullptr; if (PyDict_GetItemRef(dict.ptr(), key.ptr(), &value) < 0) [[unlikely]] { @@ -200,31 +236,31 @@ inline Py_ALWAYS_INLINE void DictSetItem(const py::handle &dict, inline Py_ALWAYS_INLINE void AssertExactList(const py::handle &object) { if (!PyList_CheckExact(object.ptr())) [[unlikely]] { - throw py::value_error("Expected an instance of list, got " + PyRepr(object) + "."); + throw py::value_error(std::format("Expected an instance of list, got {}.", object)); } } inline Py_ALWAYS_INLINE void AssertExactTuple(const py::handle &object) { if (!PyTuple_CheckExact(object.ptr())) [[unlikely]] { - throw py::value_error("Expected an instance of tuple, got " + PyRepr(object) + "."); + throw py::value_error(std::format("Expected an instance of tuple, got {}.", object)); } } inline Py_ALWAYS_INLINE void AssertExactDict(const py::handle &object) { if (!PyDict_CheckExact(object.ptr())) [[unlikely]] { - throw py::value_error("Expected an instance of dict, got " + PyRepr(object) + "."); + throw py::value_error(std::format("Expected an instance of dict, got {}.", object)); } } inline Py_ALWAYS_INLINE void AssertExactOrderedDict(const py::handle &object) { if (!py::type::handle_of(object).is(PyOrderedDictTypeObject)) [[unlikely]] { - throw py::value_error("Expected an instance of collections.OrderedDict, got " + - PyRepr(object) + "."); + throw py::value_error( + std::format("Expected an instance of collections.OrderedDict, got {}.", object)); } } inline Py_ALWAYS_INLINE void AssertExactDefaultDict(const py::handle &object) { if (!py::type::handle_of(object).is(PyDefaultDictTypeObject)) [[unlikely]] { - throw py::value_error("Expected an instance of collections.defaultdict, got " + - PyRepr(object) + "."); + throw py::value_error( + std::format("Expected an instance of collections.defaultdict, got {}.", object)); } } @@ -233,16 +269,16 @@ inline Py_ALWAYS_INLINE void AssertExactStandardDict(const py::handle &object) { py::type::handle_of(object).is(PyOrderedDictTypeObject) || py::type::handle_of(object).is(PyDefaultDictTypeObject))) [[unlikely]] { throw py::value_error( - "Expected an instance of dict, collections.OrderedDict, or collections.defaultdict, " - "got " + - PyRepr(object) + "."); + std::format("Expected an instance of dict, collections.OrderedDict, " + "or collections.defaultdict, got {}.", + object)); } } inline Py_ALWAYS_INLINE void AssertExactDeque(const py::handle &object) { if (!py::type::handle_of(object).is(PyDequeTypeObject)) [[unlikely]] { - throw py::value_error("Expected an instance of collections.deque, got " + PyRepr(object) + - "."); + throw py::value_error( + std::format("Expected an instance of collections.deque, got {}.", object)); } } @@ -490,8 +526,8 @@ constexpr std::size_t MAX_TYPE_CACHE_SIZE = 4096; } inline Py_ALWAYS_INLINE void AssertExactNamedTuple(const py::handle &object) { if (!IsNamedTupleInstance(object)) [[unlikely]] { - throw py::value_error("Expected an instance of collections.namedtuple, got " + - PyRepr(object) + "."); + throw py::value_error( + std::format("Expected an instance of collections.namedtuple, got {}.", object)); } } [[nodiscard]] inline py::tuple NamedTupleGetFields(const py::handle &object) { @@ -499,14 +535,15 @@ inline Py_ALWAYS_INLINE void AssertExactNamedTuple(const py::handle &object) { if (PyType_Check(object.ptr())) [[unlikely]] { type = object; if (!IsNamedTupleClass(type)) [[unlikely]] { - throw py::type_error("Expected a collections.namedtuple type, got " + PyRepr(object) + - "."); + throw py::type_error( + std::format("Expected a collections.namedtuple type, got {}.", object)); } } else [[likely]] { type = py::type::handle_of(object); if (!IsNamedTupleClass(type)) [[unlikely]] { - throw py::type_error("Expected an instance of collections.namedtuple type, got " + - PyRepr(object) + "."); + throw py::type_error( + std::format("Expected an instance of collections.namedtuple type, got {}.", + object)); } } return EVALUATE_WITH_LOCK_HELD(py::getattr(type, "_fields"), type); @@ -571,8 +608,8 @@ inline Py_ALWAYS_INLINE void AssertExactNamedTuple(const py::handle &object) { } inline Py_ALWAYS_INLINE void AssertExactStructSequence(const py::handle &object) { if (!IsStructSequenceInstance(object)) [[unlikely]] { - throw py::value_error("Expected an instance of PyStructSequence type, got " + - PyRepr(object) + "."); + throw py::value_error( + std::format("Expected an instance of PyStructSequence type, got {}.", object)); } } [[nodiscard]] inline py::tuple StructSequenceGetFieldsImpl(const py::handle &type) { @@ -636,13 +673,13 @@ inline Py_ALWAYS_INLINE void AssertExactStructSequence(const py::handle &object) if (PyType_Check(object.ptr())) [[unlikely]] { type = object; if (!IsStructSequenceClass(type)) [[unlikely]] { - throw py::type_error("Expected a PyStructSequence type, got " + PyRepr(object) + "."); + throw py::type_error(std::format("Expected a PyStructSequence type, got {}.", object)); } } else [[likely]] { type = py::type::handle_of(object); if (!IsStructSequenceClass(type)) [[unlikely]] { - throw py::type_error("Expected an instance of PyStructSequence type, got " + - PyRepr(object) + "."); + throw py::type_error( + std::format("Expected an instance of PyStructSequence type, got {}.", object)); } } @@ -677,8 +714,9 @@ inline void TotalOrderSort(py::list &list) { // NOLINT[runtime/references] const auto sort_key_fn = py::cpp_function([](const py::object &obj) -> py::tuple { const py::handle cls = py::type::handle_of(obj); const py::str qualname{ - EVALUATE_WITH_LOCK_HELD(PyStr(py::getattr(cls, "__module__")) + "." + - PyStr(py::getattr(cls, "__qualname__")), + EVALUATE_WITH_LOCK_HELD(std::format("{}.{}", + PyStr(py::getattr(cls, "__module__")), + PyStr(py::getattr(cls, "__qualname__"))), cls)}; return py::make_tuple(qualname, obj); }); diff --git a/include/optree/registry.h b/include/optree/registry.h index 8dcfbcb9..175813ed 100644 --- a/include/optree/registry.h +++ b/include/optree/registry.h @@ -21,6 +21,7 @@ limitations under the License. #include // std::shared_ptr #include // std::optional, std::nullopt #include // std::string +#include // std::string_view #include // std::unordered_map #include // std::unordered_set #include // std::pair, std::make_pair @@ -198,12 +199,15 @@ class PyTreeTypeRegistry { const auto interpid = GetCurrentPyInterpreterID(); const auto &namespaces = sm_dict_insertion_ordered_namespaces; + // Probe with a view: building the `std::string` half of the key would copy the namespace on + // every flatten. const bool in_current_namespace = - namespaces.find({interpid, registry_namespace}) != namespaces.end(); + namespaces.contains(std::pair{interpid, std::string_view{registry_namespace}}); return { .in_current_namespace = in_current_namespace, .with_inherited_global_namespace = - in_current_namespace || namespaces.find({interpid, ""}) != namespaces.end(), + in_current_namespace || + namespaces.contains(std::pair{interpid, std::string_view{}}), }; } @@ -259,8 +263,12 @@ class PyTreeTypeRegistry { static void Clear(); using RegistrationsMap = std::unordered_map; - using NamedRegistrationsMap = - std::unordered_map, RegistrationPtr>; + // Declared with the transparent functors explicitly: an `is_transparent` marker on a + // `std::hash` / `std::equal_to` specialization does nothing unless the container uses it. + using NamedRegistrationsMap = std::unordered_map, + RegistrationPtr, + NamespacedTypeHash, + NamespacedTypeEqual>; using BuiltinsTypesSet = std::unordered_set; RegistrationsMap m_registrations{}; @@ -269,7 +277,9 @@ class PyTreeTypeRegistry { // A set of namespaces that preserve the insertion order of the dictionary keys during // flattening. - static inline std::unordered_set> + static inline std::unordered_set, + InterpreterNamespaceHash, + InterpreterNamespaceEqual> sm_dict_insertion_ordered_namespaces{}; static inline read_write_mutex sm_dict_order_mutex{}; friend class PyTreeSpec; diff --git a/include/optree/treespec.h b/include/optree/treespec.h index 05197908..6fdcb9b7 100644 --- a/include/optree/treespec.h +++ b/include/optree/treespec.h @@ -20,6 +20,7 @@ limitations under the License. #include // std::atomic #include // std::unique_ptr #include // std::optional, std::nullopt +#include // std::span #include // std::string #include // std::thread::id #include // std::tuple @@ -325,10 +326,8 @@ class PyTreeSpec { [[nodiscard]] static std::string NodeKindToString(const Node &node); // Manufacture an instance of a node given its children. - [[nodiscard]] static py::object MakeNode( - const Node &node, - const py::object children[], // NOLINT[cppcoreguidelines-avoid-c-arrays] - const size_t &num_children); + [[nodiscard]] static py::object MakeNode(const Node &node, + const std::span &children); // Identify the path entry class for a node. [[nodiscard]] static py::object GetPathEntryType(const Node &node); diff --git a/pyproject.toml b/pyproject.toml index ed7f3af1..31a69c33 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -116,6 +116,8 @@ environment = { PYTHONDEVMODE = "1", PYTHONUNBUFFERED = "1" } environment-pass = [ "CMAKE_MINIMUM_VERSION", "CMAKE_CXX_STANDARD", + "MACOSX_DEPLOYMENT_TARGET", + "IPHONEOS_DEPLOYMENT_TARGET", "OPTREE_CXX_WERROR", "_GLIBCXX_USE_CXX11_ABI", "_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR", @@ -145,15 +147,31 @@ test-command = '''python -m pytest -Walways --color=yes --showlocals --no-cov -- environment = { PYTHONDEVMODE = "1", PYTHONUNBUFFERED = "1", CMAKE_SYSTEM_NAME = "iOS" } test-command = '''python -m pytest -Walways --color=yes --showlocals --no-cov --exitfirst tests''' +# cibuildwheel's iOS cross-build environment hardcodes an `ios-13.0-*` platform, so wheels are +# tagged `ios_13_0_*` whatever they were compiled against. Retag to the real floor: otherwise `pip` +# installs them on iOS 13.0-16.2 and `dyld` fails to load `_C`. Drop once cibuildwheel derives the +# tag from the deployment target. [[tool.cibuildwheel.overrides]] select = "*iphoneos*" inherit.environment = "append" environment = { CMAKE_OSX_SYSROOT = "iphoneos" } +repair-wheel-command = """ +python -m pip install --quiet --upgrade wheel && \ +target="$(echo "${IPHONEOS_DEPLOYMENT_TARGET:-16.3}" | tr . _)" && \ +retagged="$(python -m wheel tags --platform-tag "ios_${target}_arm64_iphoneos" --remove "{wheel}")" && \ +mv "$(dirname "{wheel}")/${retagged}" "{dest_dir}/" +""" [[tool.cibuildwheel.overrides]] select = "*iphonesimulator*" inherit.environment = "append" environment = { CMAKE_OSX_SYSROOT = "iphonesimulator" } +repair-wheel-command = """ +python -m pip install --quiet --upgrade wheel && \ +target="$(echo "${IPHONEOS_DEPLOYMENT_TARGET:-16.3}" | tr . _)" && \ +retagged="$(python -m wheel tags --platform-tag "ios_${target}_arm64_iphonesimulator" --remove "{wheel}")" && \ +mv "$(dirname "{wheel}")/${retagged}" "{dest_dir}/" +""" # Linter tools ################################################################# diff --git a/src/optree.cpp b/src/optree.cpp index e724a7ec..1e3e66ea 100644 --- a/src/optree.cpp +++ b/src/optree.cpp @@ -17,6 +17,7 @@ limitations under the License. #include "optree/optree.h" +#include // std::format #include // std::{not_,}equal_to, std::less{,_equal}, std::greater{,_equal} #include // std::unique_ptr #include // std::optional, std::nullopt @@ -48,8 +49,8 @@ void BuildModule(py::module_ &mod) { // NOLINT[runtime/references] GetCxxModule(mod); - mod.doc() = "Optimized PyTree Utilities. (C extension module built from " + - std::string(__FILE_RELPATH_FROM_PROJECT_ROOT__) + ")"; + mod.doc() = std::format("Optimized PyTree Utilities. (C extension module built from {})", + RelpathFromProjectRoot()); mod.attr("Py_TPFLAGS_BASETYPE") = py::int_(Py_TPFLAGS_BASETYPE); mod.attr("PyStructSequence_UnnamedField") = py::str(PyStructSequenceUnnamedField()); diff --git a/src/registry.cpp b/src/registry.cpp index 34fef8d6..f7629945 100644 --- a/src/registry.cpp +++ b/src/registry.cpp @@ -15,12 +15,14 @@ limitations under the License. ================================================================================ */ +#include // std::format #include // std::make_shared #include // std::optional #include // std::ostringstream #include // std::string +#include // std::string_view #include // std::remove_const_t -#include // std::move, std::make_pair +#include // std::move, std::make_pair, std::pair #include "optree/optree.h" @@ -36,8 +38,9 @@ template const auto add_builtin_type = [®istry](const py::object &cls, const PyTreeKind &kind) -> void { EXPECT_TRUE(registry.m_builtins_types.emplace(cls).second, - "PyTree type " + PyRepr(cls) + - " is already registered in the built-in types set."); + std::format("PyTree type {} is already registered " + "in the built-in types set.", + cls)); if (!NoneIsLeaf || kind != PyTreeKind::None) [[likely]] { auto registration = std::make_shared>(); @@ -45,8 +48,9 @@ template registration->type = py::reinterpret_borrow(cls); EXPECT_TRUE( registry.m_registrations.emplace(cls, std::move(registration)).second, - "PyTree type " + PyRepr(cls) + - " is already registered in the global namespace."); + std::format("PyTree type {} is already registered " + "in the global namespace.", + cls)); } if constexpr (!NoneIsLeaf) { cls.inc_ref(); @@ -90,7 +94,7 @@ PyTreeTypeRegistry::RegistryStatus PyTreeTypeRegistry::RegisterImpl( const py::function &unflatten_func, const py::object &path_entry_type, const std::string ®istry_namespace) { - if (m_builtins_types.find(cls) != m_builtins_types.end()) [[unlikely]] { + if (m_builtins_types.contains(cls)) [[unlikely]] { return RegistryStatus::BuiltinType; } @@ -166,8 +170,8 @@ PyTreeTypeRegistry::RegistryStatus PyTreeTypeRegistry::RegisterImpl( // Python bytecode, which can hand off the GIL to a thread blocking on `sm_mutex` in read mode. if (status != RegistryStatus::Ok) [[unlikely]] { if (status == RegistryStatus::BuiltinType) [[unlikely]] { - throw py::value_error("PyTree type " + PyRepr(cls) + - " is a built-in type and cannot be re-registered."); + throw py::value_error( + std::format("PyTree type {} is a built-in type and cannot be re-registered.", cls)); } std::ostringstream oss{}; oss << "PyTree type " << PyRepr(cls) << " is already registered in "; @@ -208,7 +212,7 @@ PyTreeTypeRegistry::RegistryStatus PyTreeTypeRegistry::UnregisterImpl( const py::object &cls, const std::string ®istry_namespace, RegistrationPtr ®istration) { - if (m_builtins_types.find(cls) != m_builtins_types.end()) [[unlikely]] { + if (m_builtins_types.contains(cls)) [[unlikely]] { return RegistryStatus::BuiltinType; } @@ -220,7 +224,8 @@ PyTreeTypeRegistry::RegistryStatus PyTreeTypeRegistry::UnregisterImpl( registration = it->second; m_registrations.erase(it); } else [[likely]] { - const auto named_it = m_named_registrations.find(std::make_pair(registry_namespace, cls)); + const auto named_it = m_named_registrations.find( + std::pair{std::string_view{registry_namespace}, py::handle{cls}}); if (named_it == m_named_registrations.end()) [[unlikely]] { return RegistryStatus::NotRegistered; } @@ -268,8 +273,8 @@ PyTreeTypeRegistry::RegistryStatus PyTreeTypeRegistry::UnregisterImpl( // Format the error only after the lock is released (mirrors `Register`). if (status != RegistryStatus::Ok) [[unlikely]] { if (status == RegistryStatus::BuiltinType) [[unlikely]] { - throw py::value_error("PyTree type " + PyRepr(cls) + - " is a built-in type and cannot be unregistered."); + throw py::value_error( + std::format("PyTree type {} is a built-in type and cannot be unregistered.", cls)); } std::ostringstream oss{}; oss << "PyTree type " << PyRepr(cls) << " "; @@ -308,8 +313,8 @@ template { const scoped_read_lock lock{sm_mutex}; if (!registry_namespace.empty()) [[unlikely]] { - const auto named_it = - registry.m_named_registrations.find(std::make_pair(registry_namespace, cls)); + const auto named_it = registry.m_named_registrations.find( + std::pair{std::string_view{registry_namespace}, py::handle{cls}}); if (named_it != registry.m_named_registrations.end()) [[likely]] { return named_it->second; } @@ -409,9 +414,8 @@ template PyTreeKind PyTreeTypeRegistry::GetKind( { const scoped_write_lock lock{sm_mutex}; - EXPECT_NE(sm_alive_interpids.find(interpid), - sm_alive_interpids.end(), - "The current interpreter ID should be present in the alive interpreters set."); + EXPECT_TRUE(sm_alive_interpids.contains(interpid), + "The current interpreter ID should be present in the alive interpreters set."); sm_alive_interpids.erase(interpid); { @@ -441,14 +445,14 @@ template PyTreeKind PyTreeTypeRegistry::GetKind( #if defined(Py_DEBUG) for (const auto &cls : registry1.m_builtins_types) { - EXPECT_NE(registry1.m_registrations.find(cls), registry1.m_registrations.end()); - EXPECT_NE(registry2.m_builtins_types.find(cls), registry2.m_builtins_types.end()); + EXPECT_TRUE(registry1.m_registrations.contains(cls)); + EXPECT_TRUE(registry2.m_builtins_types.contains(cls)); } for (const auto &cls : registry2.m_builtins_types) { if (cls.is(PyNoneTypeObject)) [[unlikely]] { - EXPECT_EQ(registry2.m_registrations.find(cls), registry2.m_registrations.end()); + EXPECT_FALSE(registry2.m_registrations.contains(cls)); } else [[likely]] { - EXPECT_NE(registry2.m_registrations.find(cls), registry2.m_registrations.end()); + EXPECT_TRUE(registry2.m_registrations.contains(cls)); } } for (const auto &[cls2, registration2] : registry2.m_registrations) { diff --git a/src/treespec/constructors.cpp b/src/treespec/constructors.cpp index db36f570..6e0a22cd 100644 --- a/src/treespec/constructors.cpp +++ b/src/treespec/constructors.cpp @@ -15,10 +15,10 @@ limitations under the License. ================================================================================ */ -#include // std::copy +#include // std::ranges::copy +#include // std::format #include // std::back_inserter #include // std::unique_ptr, std::make_unique -#include // std::ostringstream #include // std::runtime_error #include // std::string #include // std::move @@ -85,10 +85,9 @@ template std::vector &treespecs) -> void { for (const py::object &child : children) { if (!py::isinstance(child)) [[unlikely]] { - std::ostringstream oss{}; - oss << "Expected a(n) " << NodeKindToString(node) << " of PyTreeSpec(s), got " - << PyRepr(handle) << "."; - throw py::value_error(oss.str()); + throw py::value_error(std::format("Expected a(n) {} of PyTreeSpec(s), got {}.", + NodeKindToString(node), + handle)); } treespecs.emplace_back(thread_safe_cast(child)); } @@ -105,11 +104,10 @@ template if (common_registry_namespace.empty()) [[likely]] { common_registry_namespace = treespec.m_namespace; } else if (common_registry_namespace != treespec.m_namespace) [[unlikely]] { - std::ostringstream oss{}; - oss << "Expected treespecs with the same namespace, got " - << PyRepr(common_registry_namespace) << " vs. " - << PyRepr(treespec.m_namespace) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Expected treespecs with the same namespace, got {} vs. {}.", + PyRepr(common_registry_namespace), + PyRepr(treespec.m_namespace))); } } } @@ -123,10 +121,9 @@ template if (registry_namespace.empty()) [[likely]] { registry_namespace = common_registry_namespace; } else if (registry_namespace != common_registry_namespace) [[unlikely]] { - std::ostringstream oss{}; - oss << "Expected treespec(s) with namespace " << PyRepr(registry_namespace) - << ", got " << PyRepr(common_registry_namespace) << "."; - throw py::value_error(oss.str()); + throw py::value_error(std::format("Expected treespec(s) with namespace {}, got {}.", + PyRepr(registry_namespace), + PyRepr(common_registry_namespace))); } } else if (!depends_on_namespace) [[likely]] { registry_namespace = ""; // mirrors `Flatten` @@ -239,10 +236,11 @@ template node.custom->flatten_func); const ssize_t num_out = TupleGetSize(out); if (num_out != 2 && num_out != 3) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTree custom flatten function for type " << PyRepr(node.custom->type) - << " should return a 2- or 3-tuple, got " << num_out << "."; - throw std::runtime_error(oss.str()); + throw std::runtime_error( + std::format("PyTree custom flatten function for type {} should return a 2- or " + "3-tuple, got {}.", + node.custom->type, + num_out)); } node.arity = 0; node.node_data = TupleGetItem(out, 1); @@ -261,12 +259,13 @@ template node.node_entries = thread_safe_cast(node_entries); const ssize_t num_entries = TupleGetSize(node.node_entries); if (num_entries != node.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTree custom flatten function for type " - << PyRepr(node.custom->type) - << " returned inconsistent number of children (" << node.arity - << ") and number of entries (" << num_entries << ")."; - throw std::runtime_error(oss.str()); + throw std::runtime_error( + std::format("PyTree custom flatten function for type {} returned " + "inconsistent number of children ({}) " + "and number of entries ({}).", + node.custom->type, + node.arity, + num_entries)); } } } @@ -281,9 +280,7 @@ template auto out = std::make_unique(); ssize_t num_leaves = ((node.kind == PyTreeKind::Leaf) ? 1 : 0); for (const PyTreeSpec &treespec : treespecs) { - std::copy(treespec.m_traversal.cbegin(), - treespec.m_traversal.cend(), - std::back_inserter(out->m_traversal)); + std::ranges::copy(treespec.m_traversal, std::back_inserter(out->m_traversal)); num_leaves += treespec.GetNumLeaves(); } node.num_leaves = num_leaves; @@ -299,12 +296,11 @@ template // which resolves every custom node globally. if (!registry_namespace.empty()) [[unlikely]] { if (const auto stale_type = out->FindStaleCustomType(registry_namespace)) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTreeSpecs cannot be composed into a collection: custom PyTree type " - << PyRepr(*stale_type) - << " no longer resolves to its original registration in namespace " - << PyRepr(registry_namespace) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("PyTreeSpecs cannot be composed into a collection: custom PyTree type " + "{} no longer resolves to its original registration in namespace {}.", + *stale_type, + PyRepr(registry_namespace))); } } out->m_traversal.shrink_to_fit(); diff --git a/src/treespec/flatten.cpp b/src/treespec/flatten.cpp index 0aff13dc..c240b8df 100644 --- a/src/treespec/flatten.cpp +++ b/src/treespec/flatten.cpp @@ -15,6 +15,7 @@ limitations under the License. ================================================================================ */ +#include // std::format #include // std::unique_ptr, std::make_unique #include // std::optional #include // std::ostringstream @@ -154,10 +155,11 @@ bool PyTreeSpec::FlattenIntoImpl(const py::handle &handle, node.custom->flatten_func); const ssize_t num_out = TupleGetSize(out); if (num_out != 2 && num_out != 3) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTree custom flatten function for type " << PyRepr(node.custom->type) - << " should return a 2- or 3-tuple, got " << num_out << "."; - throw std::runtime_error(oss.str()); + throw std::runtime_error( + std::format("PyTree custom flatten function for type {} should " + "return a 2- or 3-tuple, got {}.", + node.custom->type, + num_out)); } node.arity = 0; node.node_data = TupleGetItem(out, 1); @@ -175,12 +177,13 @@ bool PyTreeSpec::FlattenIntoImpl(const py::handle &handle, node.node_entries = thread_safe_cast(node_entries); const ssize_t num_entries = TupleGetSize(node.node_entries); if (num_entries != node.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTree custom flatten function for type " - << PyRepr(node.custom->type) - << " returned inconsistent number of children (" << node.arity - << ") and number of entries (" << num_entries << ")."; - throw std::runtime_error(oss.str()); + throw std::runtime_error( + std::format("PyTree custom flatten function for type {} returned " + "inconsistent number of children ({}) " + "and number of entries ({}).", + node.custom->type, + node.arity, + num_entries)); } } } @@ -337,7 +340,7 @@ bool PyTreeSpec::FlattenIntoWithPathImpl(const py::handle &handle, } INTERNAL_ERROR( "NoneIsLeaf is true, but PyTreeTypeRegistry::GetKind() returned " - "PyTreeKind::None`."); + "`PyTreeKind::None`."); } case PyTreeKind::Tuple: { @@ -411,10 +414,11 @@ bool PyTreeSpec::FlattenIntoWithPathImpl(const py::handle &handle, node.custom->flatten_func); const ssize_t num_out = TupleGetSize(out); if (num_out != 2 && num_out != 3) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTree custom flatten function for type " << PyRepr(node.custom->type) - << " should return a 2- or 3-tuple, got " << num_out << "."; - throw std::runtime_error(oss.str()); + throw std::runtime_error( + std::format("PyTree custom flatten function for type {} should " + "return a 2- or 3-tuple, got {}.", + node.custom->type, + num_out)); } node.arity = 0; node.node_data = TupleGetItem(out, 1); @@ -438,20 +442,21 @@ bool PyTreeSpec::FlattenIntoWithPathImpl(const py::handle &handle, const scoped_critical_section cs{children}; for (const py::handle &child : children) { if (num_children >= node.arity) [[unlikely]] { - throw std::runtime_error( - "PyTree custom flatten function for type " + - PyRepr(node.custom->type) + - " returned inconsistent number of children and number of entries."); + throw std::runtime_error(std::format( + "PyTree custom flatten function for type {} returned " + "inconsistent number of children and number of entries.", + node.custom->type)); } recurse(child, TupleGetItem(node.node_entries, num_children++)); } if (num_children != node.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTree custom flatten function for type " - << PyRepr(node.custom->type) - << " returned inconsistent number of children (" << num_children - << ") and number of entries (" << node.arity << ")."; - throw std::runtime_error(oss.str()); + throw std::runtime_error( + std::format("PyTree custom flatten function for type {} returned " + "inconsistent number of children ({}) " + "and number of entries ({}).", + node.custom->type, + num_children, + node.arity)); } } break; @@ -563,10 +568,10 @@ py::list PyTreeSpec::FlattenUpTo(const py::object &tree) const { auto leaves = reserved_vector(num_leaves); while (!agenda.empty()) [[likely]] { if (it == m_traversal.crend()) [[unlikely]] { - std::ostringstream oss{}; - oss << "Tree structures did not match; expected: " << ToString() - << ", got: " << PyRepr(tree) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Tree structures did not match; expected: {}, got: {}.", + ToString(), + tree)); } const Node &node = *it; const py::object object = std::move(agenda.back()); @@ -587,9 +592,7 @@ py::list PyTreeSpec::FlattenUpTo(const py::object &tree) const { "`PyTreeKind::None`."); } if (!object.is_none()) [[likely]] { - std::ostringstream oss{}; - oss << "Expected None, got " << PyRepr(object) << "."; - throw py::value_error(oss.str()); + throw py::value_error(std::format("Expected None, got {}.", object)); } break; } @@ -598,10 +601,11 @@ py::list PyTreeSpec::FlattenUpTo(const py::object &tree) const { AssertExactTuple(object); const auto tuple = py::reinterpret_borrow(object); if (TupleGetSize(tuple) != node.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "tuple arity mismatch; expected: " << node.arity - << ", got: " << TupleGetSize(tuple) << "; tuple: " << PyRepr(object) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("tuple arity mismatch; expected: {}, got: {}; tuple: {}.", + node.arity, + TupleGetSize(tuple), + object)); } for (ssize_t i = 0; i < node.arity; ++i) { agenda.emplace_back(TupleGetItem(tuple, i)); @@ -614,10 +618,11 @@ py::list PyTreeSpec::FlattenUpTo(const py::object &tree) const { const scoped_critical_section cs{object}; const auto list = py::reinterpret_borrow(object); if (ListGetSize(list) != node.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "list arity mismatch; expected: " << node.arity - << ", got: " << ListGetSize(list) << "; list: " << PyRepr(object) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("list arity mismatch; expected: {}, got: {}; list: {}.", + node.arity, + ListGetSize(list), + object)); } for (ssize_t i = 0; i < node.arity; ++i) { agenda.emplace_back(ListGetItem(list, i)); @@ -645,18 +650,13 @@ py::list PyTreeSpec::FlattenUpTo(const py::object &tree) const { if (ListGetSize(extra_keys) != 0) [[likely]] { key_difference_sstream << ", extra key(s): " << PyRepr(extra_keys); } - std::ostringstream oss{}; - oss << "dictionary key mismatch; expected key(s): " << PyRepr(expected_keys) - << ", got key(s): " << PyRepr(keys) << key_difference_sstream.str() << "; "; - if (node.kind == PyTreeKind::Dict) [[likely]] { - oss << "dict"; - } else if (node.kind == PyTreeKind::OrderedDict) [[likely]] { - oss << "OrderedDict"; - } else [[unlikely]] { - oss << "defaultdict"; - } - oss << ": " << PyRepr(object) << "."; - throw py::value_error(oss.str()); + throw py::value_error(std::format( + "dictionary key mismatch; expected key(s): {}, got key(s): {}{}; {}: {}.", + py::handle{expected_keys}, + py::handle{keys}, + key_difference_sstream.str(), + NodeKindToString(node), + object)); } for (const py::handle &key : expected_keys) { agenda.emplace_back(DictGetItem(dict, key)); @@ -668,17 +668,19 @@ py::list PyTreeSpec::FlattenUpTo(const py::object &tree) const { AssertExactNamedTuple(object); const auto tuple = py::reinterpret_borrow(object); if (TupleGetSize(tuple) != node.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "namedtuple arity mismatch; expected: " << node.arity - << ", got: " << TupleGetSize(tuple) << "; tuple: " << PyRepr(object) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("namedtuple arity mismatch; expected: {}, got: {}; tuple: {}.", + node.arity, + TupleGetSize(tuple), + object)); } if (py::type::handle_of(object).not_equal(node.node_data)) [[unlikely]] { - std::ostringstream oss{}; - oss << "namedtuple type mismatch; expected type: " << PyRepr(node.node_data) - << ", got type: " << PyRepr(py::type::handle_of(object)) - << "; tuple: " << PyRepr(object) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("namedtuple type mismatch; expected type: {}, got type: {}; " + "tuple: {}.", + node.node_data, + py::type::handle_of(object), + object)); } for (ssize_t i = 0; i < node.arity; ++i) { agenda.emplace_back(TupleGetItem(tuple, i)); @@ -690,10 +692,11 @@ py::list PyTreeSpec::FlattenUpTo(const py::object &tree) const { AssertExactDeque(object); const auto list = thread_safe_cast(object); if (ListGetSize(list) != node.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "deque arity mismatch; expected: " << node.arity - << ", got: " << ListGetSize(list) << "; deque: " << PyRepr(object) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("deque arity mismatch; expected: {}, got: {}; deque: {}.", + node.arity, + ListGetSize(list), + object)); } for (ssize_t i = 0; i < node.arity; ++i) { agenda.emplace_back(ListGetItem(list, i)); @@ -705,18 +708,20 @@ py::list PyTreeSpec::FlattenUpTo(const py::object &tree) const { AssertExactStructSequence(object); const auto tuple = py::reinterpret_borrow(object); if (TupleGetSize(tuple) != node.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyStructSequence arity mismatch; expected: " << node.arity - << ", got: " << TupleGetSize(tuple) << "; tuple: " << PyRepr(object) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("PyStructSequence arity mismatch; expected: {}, got: {}; " + "tuple: {}.", + node.arity, + TupleGetSize(tuple), + object)); } if (py::type::handle_of(object).not_equal(node.node_data)) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyStructSequence type mismatch; expected type: " - << PyRepr(node.node_data) - << ", got type: " << PyRepr(py::type::handle_of(object)) - << "; tuple: " << PyRepr(object) << "."; - throw py::value_error(oss.str()); + throw py::value_error(std::format( + "PyStructSequence type mismatch; expected type: {}, got type: {}; " + "tuple: {}.", + node.node_data, + py::type::handle_of(object), + object)); } for (ssize_t i = 0; i < node.arity; ++i) { agenda.emplace_back(TupleGetItem(tuple, i)); @@ -734,11 +739,12 @@ py::list PyTreeSpec::FlattenUpTo(const py::object &tree) const { PyTreeTypeRegistry::Lookup(py::type::of(object), m_namespace); } if (registration != node.custom) [[unlikely]] { - std::ostringstream oss{}; - oss << "Custom node type mismatch; expected type: " << PyRepr(node.custom->type) - << ", got type: " << PyRepr(py::type::handle_of(object)) - << "; value: " << PyRepr(object) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Custom node type mismatch; expected type: {}, got type: {}; " + "value: {}.", + node.custom->type, + py::type::handle_of(object), + object)); } const py::tuple out = EVALUATE_WITH_LOCK_HELD2( thread_safe_cast(node.custom->flatten_func(object)), @@ -746,20 +752,22 @@ py::list PyTreeSpec::FlattenUpTo(const py::object &tree) const { node.custom->flatten_func); const ssize_t num_out = TupleGetSize(out); if (num_out != 2 && num_out != 3) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTree custom flatten function for type " << PyRepr(node.custom->type) - << " should return a 2- or 3-tuple, got " << num_out << "."; - throw std::runtime_error(oss.str()); + throw std::runtime_error( + std::format("PyTree custom flatten function for type {} should " + "return a 2- or 3-tuple, got {}.", + node.custom->type, + num_out)); } { const py::object node_data = TupleGetItem(out, 1); const scoped_critical_section2 cs{node.node_data, node_data}; if (node.node_data.not_equal(node_data)) [[unlikely]] { - std::ostringstream oss{}; - oss << "Mismatch custom node data; expected: " << PyRepr(node.node_data) - << ", got: " << PyRepr(node_data) << "; value: " << PyRepr(object) - << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Mismatch custom node data; expected: {}, got: {}; " + "value: {}.", + node.node_data, + node_data, + object)); } } ssize_t arity = 0; @@ -772,10 +780,11 @@ py::list PyTreeSpec::FlattenUpTo(const py::object &tree) const { } } if (arity != node.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "Custom type arity mismatch; expected: " << node.arity - << ", got: " << arity << "; value: " << PyRepr(object) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Custom type arity mismatch; expected: {}, got: {}; value: {}.", + node.arity, + arity, + object)); } break; } @@ -786,10 +795,8 @@ py::list PyTreeSpec::FlattenUpTo(const py::object &tree) const { } } if (it != m_traversal.crend() || py::ssize_t_cast(leaves.size()) != num_leaves) [[unlikely]] { - std::ostringstream oss{}; - oss << "Tree structures did not match; expected: " << ToString() - << ", got: " << PyRepr(tree) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Tree structures did not match; expected: {}, got: {}.", ToString(), tree)); } py::list result{num_leaves}; ssize_t index = num_leaves; diff --git a/src/treespec/hashing.cpp b/src/treespec/hashing.cpp index 1040201f..a0faaf9b 100644 --- a/src/treespec/hashing.cpp +++ b/src/treespec/hashing.cpp @@ -102,7 +102,7 @@ ssize_t PyTreeSpec::HashValue() const { const ThreadedIdentity ident{this, std::this_thread::get_id()}; { const scoped_read_lock lock{mutex}; - if (running.find(ident) != running.end()) [[unlikely]] { + if (running.contains(ident)) [[unlikely]] { return 0; } } diff --git a/src/treespec/richcomparison.cpp b/src/treespec/richcomparison.cpp index 42b60d2d..77170512 100644 --- a/src/treespec/richcomparison.cpp +++ b/src/treespec/richcomparison.cpp @@ -15,7 +15,7 @@ limitations under the License. ================================================================================ */ -#include // std::copy, std::reverse +#include // std::ranges::copy, std::ranges::reverse #include // std::make_reverse_iterator #include // std::unordered_map #include // std::vector @@ -104,8 +104,8 @@ bool PyTreeSpec::IsPrefix(const PyTreeSpec &other, const bool &strict) const { other_offsets.emplace_back(other_offsets.back() + num_nodes); other_cur += num_nodes; } - std::reverse(other_num_nodes.begin(), other_num_nodes.end()); - std::reverse(other_offsets.begin(), other_offsets.end()); + std::ranges::reverse(other_num_nodes); + std::ranges::reverse(other_offsets); EXPECT_EQ(other_offsets.front(), b->num_nodes, "PyTreeSpec traversal out of range."); @@ -126,7 +126,7 @@ bool PyTreeSpec::IsPrefix(const PyTreeSpec &other, const bool &strict) const { reordered_other_offsets.emplace_back(reordered_other_offsets.back() + reordered_other_num_nodes[i]); } - std::reverse(reordered_other_offsets.begin(), reordered_other_offsets.end()); + std::ranges::reverse(reordered_other_offsets); EXPECT_EQ(reordered_other_offsets.front(), b->num_nodes, "PyTreeSpec traversal out of range."); @@ -138,9 +138,9 @@ bool PyTreeSpec::IsPrefix(const PyTreeSpec &other, const bool &strict) const { const std::vector b_subtree(b.base() - b->num_nodes, b.base()); const auto original_b = std::make_reverse_iterator(b_subtree.cend()); for (const auto &[i, j] : reordered_index_to_index) { - std::copy(original_b + other_offsets[j + 1], - original_b + other_offsets[j], - b + reordered_other_offsets[i + 1]); + std::ranges::copy(original_b + other_offsets[j + 1], + original_b + other_offsets[j], + b + reordered_other_offsets[i + 1]); } } break; diff --git a/src/treespec/serialization.cpp b/src/treespec/serialization.cpp index 29123565..78091e49 100644 --- a/src/treespec/serialization.cpp +++ b/src/treespec/serialization.cpp @@ -16,13 +16,14 @@ limitations under the License. */ #include // std::rethrow_exception, std::current_exception +#include // std::format #include // std::unique_ptr, std::make_unique #include // std::ostringstream #include // std::runtime_error #include // std::string #include // std::this_thread::get_id #include // std::unordered_set -#include // std::pair, std::move +#include // std::cmp_less, std::pair, std::move #include "optree/optree.h" @@ -142,12 +143,13 @@ std::string PyTreeSpec::ToStringImpl() const { // a caller may have changed them after the treespec was built. Report the mismatch // as a `ValueError`, not an internal error, since the cause is external. if (TupleGetSize(fields) != node.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "Number of fields (" << TupleGetSize(fields) << ") of namedtuple type " - << PyRepr(type) << " does not match the arity (" << node.arity - << ") of the treespec node. The `_fields` attribute may have been modified " - "after the treespec was created."; - throw py::value_error(oss.str()); + throw py::value_error(std::format( + "Number of fields ({}) of namedtuple type {} does not match the arity ({}) " + "of the treespec node. The `_fields` attribute may have been modified " + "after the treespec was created.", + TupleGetSize(fields), + type, + node.arity)); } const std::string kind = PyStr(EVALUATE_WITH_LOCK_HELD(py::getattr(type, "__name__"), type)); @@ -288,7 +290,7 @@ std::string PyTreeSpec::ToString() const { const ThreadedIdentity ident{this, std::this_thread::get_id()}; { const scoped_read_lock lock{mutex}; - if (running.find(ident) != running.end()) [[unlikely]] { + if (running.contains(ident)) [[unlikely]] { return "..."; } } @@ -356,7 +358,7 @@ py::object PyTreeSpec::ToPicklable() const { // NOLINTNEXTLINE[readability-function-cognitive-complexity] /*static*/ std::unique_ptr PyTreeSpec::FromPicklable(const py::object &picklable) { const auto malformed = [](const std::string &reason) -> std::runtime_error { - return std::runtime_error("Malformed pickled PyTreeSpec: " + reason + "."); + return std::runtime_error(std::format("Malformed pickled PyTreeSpec: {}.", reason)); }; // `DistinctCount` hashes the keys, so an unhashable one raises `TypeError`. Report it as a // malformed pickle like every other structural defect instead of letting it escape. @@ -606,7 +608,7 @@ py::object PyTreeSpec::ToPicklable() const { reserved_vector>( out->m_traversal.size()); for (const Node &node : out->m_traversal) { - if (static_cast(subtree_sizes.size()) < node.arity) [[unlikely]] { + if (std::cmp_less(subtree_sizes.size(), node.arity)) [[unlikely]] { throw malformed("a node has more children than available subtrees"); } ssize_t children_num_nodes = 0; diff --git a/src/treespec/traversal.cpp b/src/treespec/traversal.cpp index 8422b780..9d910460 100644 --- a/src/treespec/traversal.cpp +++ b/src/treespec/traversal.cpp @@ -15,10 +15,11 @@ limitations under the License. ================================================================================ */ -#include // std::memory_order_acquire, std::memory_order_release +#include // std::memory_order #include // std::rethrow_exception, std::current_exception +#include // std::format #include // std::optional -#include // std::ostringstream +#include // std::span #include // std::runtime_error #include // std::this_thread::get_id, std::thread::id @@ -126,10 +127,11 @@ py::object PyTreeIter::NextImpl() { custom->flatten_func); const ssize_t num_out = TupleGetSize(out); if (num_out != 2 && num_out != 3) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTree custom flatten function for type " << PyRepr(custom->type) - << " should return a 2- or 3-tuple, got " << num_out << "."; - throw std::runtime_error(oss.str()); + throw std::runtime_error( + std::format("PyTree custom flatten function for type {} should " + "return a 2- or 3-tuple, got {}.", + custom->type, + num_out)); } auto children = thread_safe_cast(TupleGetItem(out, 0)); const ssize_t arity = TupleGetSize(children); @@ -139,12 +141,13 @@ py::object PyTreeIter::NextImpl() { const ssize_t num_entries = TupleGetSize(thread_safe_cast(node_entries)); if (num_entries != arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTree custom flatten function for type " - << PyRepr(custom->type) - << " returned inconsistent number of children (" << arity - << ") and number of entries (" << num_entries << ")."; - throw std::runtime_error(oss.str()); + throw std::runtime_error( + std::format("PyTree custom flatten function for type {} returned " + "inconsistent number of children ({}) and " + "number of entries ({}).", + custom->type, + arity, + num_entries)); } } } @@ -168,7 +171,7 @@ py::object PyTreeIter::Next() { // `next()` on this same iterator. `m_mutex` is not recursive and the GIL is released while // waiting on it, so that would hang; reject it as CPython's "generator already executing" does. const auto ident = std::this_thread::get_id(); - if (m_running_thread_id.load(std::memory_order_acquire) == ident) [[unlikely]] { + if (m_running_thread_id.load(std::memory_order::acquire) == ident) [[unlikely]] { throw std::runtime_error("PyTreeIter is already iterating."); } @@ -180,7 +183,7 @@ py::object PyTreeIter::Next() { #if !defined(Py_GIL_DISABLED) const py::gil_scoped_acquire_simple gil_acquire{}; #endif - m_running_thread_id.store(ident, std::memory_order_release); + m_running_thread_id.store(ident, std::memory_order::release); try { py::object leaf{}; if (m_none_is_leaf) [[unlikely]] { @@ -188,10 +191,10 @@ py::object PyTreeIter::Next() { } else [[likely]] { leaf = NextImpl(); } - m_running_thread_id.store(std::thread::id{}, std::memory_order_release); + m_running_thread_id.store(std::thread::id{}, std::memory_order::release); return leaf; } catch (...) { - m_running_thread_id.store(std::thread::id{}, std::memory_order_release); + m_running_thread_id.store(std::thread::id{}, std::memory_order::release); std::rethrow_exception(std::current_exception()); } } @@ -254,8 +257,8 @@ py::object PyTreeSpec::WalkImpl(const py::iterable &leaves, } else [[unlikely]] { const py::object out = MakeNode(node, - node.arity > 0 ? &agenda[size - node.arity] : nullptr, - node.arity); + node.arity > 0 ? std::span(&agenda[size - node.arity], node.arity) + : std::span{}); agenda.resize(size - node.arity); agenda.emplace_back( f_node ? EVALUATE_WITH_LOCK_HELD2((*f_node)(out), out, *f_node) : out); diff --git a/src/treespec/treespec.cpp b/src/treespec/treespec.cpp index d884dfda..aba3825d 100644 --- a/src/treespec/treespec.cpp +++ b/src/treespec/treespec.cpp @@ -15,10 +15,12 @@ limitations under the License. ================================================================================ */ -#include // std::copy, std::reverse +#include // std::ranges::copy, std::ranges::reverse +#include // std::format #include // std::back_inserter #include // std::unique_ptr, std::make_unique #include // std::optional +#include // std::span #include // std::ostringstream #include // std::string #include // std::tuple @@ -31,11 +33,9 @@ namespace optree { // NOLINTNEXTLINE[readability-function-cognitive-complexity] /*static*/ py::object PyTreeSpec::MakeNode(const Node &node, - // NOLINTNEXTLINE[cppcoreguidelines-avoid-c-arrays] - const py::object children[], - const size_t &num_children) { - EXPECT_EQ(py::ssize_t_cast(num_children), node.arity, "Node arity did not match."); - EXPECT_TRUE(children != nullptr || num_children == 0, "Node children is null."); + const std::span &children) { + EXPECT_EQ(py::ssize_t_cast(children.size()), node.arity, "Node arity did not match."); + EXPECT_TRUE(children.data() != nullptr || children.empty(), "Node children is null."); switch (node.kind) { case PyTreeKind::Leaf: @@ -50,7 +50,6 @@ namespace optree { case PyTreeKind::StructSequence: { py::tuple tuple{node.arity}; for (ssize_t i = 0; i < node.arity; ++i) { - // NOLINTNEXTLINE[cppcoreguidelines-pro-bounds-pointer-arithmetic] TupleSetItem(tuple, i, children[i]); } if (node.kind == PyTreeKind::NamedTuple) [[unlikely]] { @@ -68,7 +67,6 @@ namespace optree { case PyTreeKind::Deque: { py::list list{node.arity}; for (ssize_t i = 0; i < node.arity; ++i) { - // NOLINTNEXTLINE[cppcoreguidelines-pro-bounds-pointer-arithmetic] ListSetItem(list, i, children[i]); } if (node.kind == PyTreeKind::Deque) [[unlikely]] { @@ -97,7 +95,6 @@ namespace optree { } } for (ssize_t i = 0; i < node.arity; ++i) { - // NOLINTNEXTLINE[cppcoreguidelines-pro-bounds-pointer-arithmetic] DictSetItem(dict, ListGetItem(keys, i), children[i]); } if (node.kind == PyTreeKind::OrderedDict) [[unlikely]] { @@ -115,7 +112,6 @@ namespace optree { case PyTreeKind::Custom: { const py::tuple tuple{node.arity}; for (ssize_t i = 0; i < node.arity; ++i) { - // NOLINTNEXTLINE[cppcoreguidelines-pro-bounds-pointer-arithmetic] TupleSetItem(tuple, i, children[i]); } return EVALUATE_WITH_LOCK_HELD2(node.custom->unflatten_func(node.node_data, tuple), @@ -230,25 +226,25 @@ std::optional PyTreeSpec::FindStaleCustomType( ssize_t other_cur = other_pos - 1; if (root.kind == PyTreeKind::Leaf) [[likely]] { - std::copy(other_traversal.crend() - (other_pos + 1), - other_traversal.crend() - (other_pos - other_root.num_nodes + 1), - std::back_inserter(nodes)); + std::ranges::copy(other_traversal.crend() - (other_pos + 1), + other_traversal.crend() - (other_pos - other_root.num_nodes + 1), + std::back_inserter(nodes)); other_cur -= other_root.num_nodes - 1; return {pos - cur, other_pos - other_cur, other_root.num_nodes, other_root.num_leaves}; } if (other_root.kind == PyTreeKind::Leaf) [[likely]] { - std::copy(traversal.crend() - (pos + 1), - traversal.crend() - (pos - root.num_nodes + 1), - std::back_inserter(nodes)); + std::ranges::copy(traversal.crend() - (pos + 1), + traversal.crend() - (pos - root.num_nodes + 1), + std::back_inserter(nodes)); cur -= root.num_nodes - 1; return {pos - cur, other_pos - other_cur, root.num_nodes, root.num_leaves}; } if (root.kind == PyTreeKind::None) [[unlikely]] { if (other_root.kind != PyTreeKind::None) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTreeSpecs have incompatible node types; expected type: " - << NodeKindToString(root) << ", got: " << NodeKindToString(other_root) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("PyTreeSpecs have incompatible node types; expected type: {}, got: {}.", + NodeKindToString(root), + NodeKindToString(other_root))); } nodes.emplace_back(root); @@ -270,16 +266,16 @@ std::optional PyTreeSpec::FindStaleCustomType( case PyTreeKind::List: case PyTreeKind::Deque: { if (root.kind != other_root.kind) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTreeSpecs have incompatible node types; expected type: " - << NodeKindToString(root) << ", got: " << NodeKindToString(other_root) << "."; - throw py::value_error(oss.str()); + throw py::value_error(std::format( + "PyTreeSpecs have incompatible node types; expected type: {}, got: {}.", + NodeKindToString(root), + NodeKindToString(other_root))); } if (root.arity != other_root.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << NodeKindToString(root) << " arity mismatch; expected: " << root.arity - << ", got: " << other_root.arity << "."; - throw py::value_error(oss.str()); + throw py::value_error(std::format("{} arity mismatch; expected: {}, got: {}.", + NodeKindToString(root), + root.arity, + other_root.arity)); } break; } @@ -289,10 +285,10 @@ std::optional PyTreeSpec::FindStaleCustomType( case PyTreeKind::DefaultDict: { if (other_root.kind != PyTreeKind::Dict && other_root.kind != PyTreeKind::OrderedDict && other_root.kind != PyTreeKind::DefaultDict) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTreeSpecs have incompatible node types; expected type: " - << NodeKindToString(root) << ", got: " << NodeKindToString(other_root) << "."; - throw py::value_error(oss.str()); + throw py::value_error(std::format( + "PyTreeSpecs have incompatible node types; expected type: {}, got: {}.", + NodeKindToString(root), + NodeKindToString(other_root))); } const scoped_critical_section2 cs{root.node_data, other_root.node_data}; @@ -320,11 +316,11 @@ std::optional PyTreeSpec::FindStaleCustomType( if (ListGetSize(extra_keys) != 0) [[likely]] { key_difference_sstream << ", extra key(s): " << PyRepr(extra_keys); } - std::ostringstream oss{}; - oss << "dictionary key mismatch; expected key(s): " << PyRepr(expected_keys) - << ", got key(s): " << PyRepr(sorted_other_keys) << key_difference_sstream.str() - << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("dictionary key mismatch; expected key(s): {}, got key(s): {}{}.", + py::handle{expected_keys}, + py::handle{sorted_other_keys}, + key_difference_sstream.str())); } const size_t start_num_nodes = nodes.size(); @@ -334,7 +330,7 @@ std::optional PyTreeSpec::FindStaleCustomType( other_curs.emplace_back(other_cur); other_cur -= other_traversal.at(other_cur).num_nodes; } - std::reverse(other_curs.begin(), other_curs.end()); + std::ranges::reverse(other_curs); const ssize_t last_other_cur = other_cur; for (ssize_t i = root.arity - 1; i >= 0; --i) { const py::object key = ListGetItem(expected_keys, i); @@ -360,54 +356,54 @@ std::optional PyTreeSpec::FindStaleCustomType( case PyTreeKind::NamedTuple: case PyTreeKind::StructSequence: { if (root.kind != other_root.kind) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTreeSpecs have incompatible node types; expected type: " - << NodeKindToString(root) << ", got: " << NodeKindToString(other_root) << "."; - throw py::value_error(oss.str()); + throw py::value_error(std::format( + "PyTreeSpecs have incompatible node types; expected type: {}, got: {}.", + NodeKindToString(root), + NodeKindToString(other_root))); } if (root.arity != other_root.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << (root.kind == PyTreeKind::NamedTuple ? "namedtuple" : "PyStructSequence") - << " arity mismatch; expected: " << root.arity << ", got: " << other_root.arity - << "."; - throw py::value_error(oss.str()); + throw py::value_error(std::format( + "{} arity mismatch; expected: {}, got: {}.", + root.kind == PyTreeKind::NamedTuple ? "namedtuple" : "PyStructSequence", + root.arity, + other_root.arity)); } if (root.node_data.not_equal(other_root.node_data)) [[unlikely]] { - std::ostringstream oss{}; - oss << (root.kind == PyTreeKind::NamedTuple ? "namedtuple" : "PyStructSequence") - << " type mismatch; expected type: " << NodeKindToString(root) - << ", got type: " << NodeKindToString(other_root) << "."; - throw py::value_error(oss.str()); + throw py::value_error(std::format( + "{} type mismatch; expected type: {}, got type: {}.", + root.kind == PyTreeKind::NamedTuple ? "namedtuple" : "PyStructSequence", + NodeKindToString(root), + NodeKindToString(other_root))); } break; } case PyTreeKind::Custom: { if (root.kind != other_root.kind) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTreeSpecs have incompatible node types; expected type: " - << NodeKindToString(root) << ", got: " << NodeKindToString(other_root) << "."; - throw py::value_error(oss.str()); + throw py::value_error(std::format( + "PyTreeSpecs have incompatible node types; expected type: {}, got: {}.", + NodeKindToString(root), + NodeKindToString(other_root))); } if (!root.custom->type.is(other_root.custom->type)) [[unlikely]] { - std::ostringstream oss{}; - oss << "Custom node type mismatch; expected type: " << NodeKindToString(root) - << ", got type: " << NodeKindToString(other_root) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Custom node type mismatch; expected type: {}, got type: {}.", + NodeKindToString(root), + NodeKindToString(other_root))); } if (root.arity != other_root.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "Custom type arity mismatch; expected: " << root.arity - << ", got: " << other_root.arity << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Custom type arity mismatch; expected: {}, got: {}.", + root.arity, + other_root.arity)); } { const scoped_critical_section2 cs{root.node_data, other_root.node_data}; if (root.node_data.not_equal(other_root.node_data)) [[unlikely]] { - std::ostringstream oss{}; - oss << "Mismatch custom node data; expected: " << PyRepr(root.node_data) - << ", got: " << PyRepr(other_root.node_data) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Mismatch custom node data; expected: {}, got: {}.", + root.node_data, + other_root.node_data)); } } break; @@ -452,10 +448,10 @@ std::unique_ptr PyTreeSpec::BroadcastToCommonSuffix(const PyTreeSpec } if (!m_namespace.empty() && !other.m_namespace.empty() && m_namespace != other.m_namespace) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTreeSpecs must have the same namespace, got " << PyRepr(m_namespace) << " vs. " - << PyRepr(other.m_namespace) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("PyTreeSpecs must have the same namespace, got {} vs. {}.", + PyRepr(m_namespace), + PyRepr(other.m_namespace))); } const std::string &target_namespace = m_namespace.empty() ? other.m_namespace : m_namespace; @@ -479,11 +475,11 @@ std::unique_ptr PyTreeSpec::BroadcastToCommonSuffix(const PyTreeSpec stale_type = other.FindStaleCustomType(target_namespace); } if (stale_type) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTreeSpecs cannot be merged: custom PyTree type " << PyRepr(*stale_type) - << " no longer resolves to its original registration in namespace " - << PyRepr(target_namespace) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("PyTreeSpecs cannot be merged: custom PyTree type {} no longer " + "resolves to its original registration in namespace {}.", + *stale_type, + PyRepr(target_namespace))); } } else [[unlikely]] { EXPECT_TRUE(m_namespace.empty(), "Namespace mismatch."); @@ -500,7 +496,7 @@ std::unique_ptr PyTreeSpec::BroadcastToCommonSuffix(const PyTreeSpec other.m_traversal, other_num_nodes - 1, 0); - std::reverse(treespec->m_traversal.begin(), treespec->m_traversal.end()); + std::ranges::reverse(treespec->m_traversal); EXPECT_EQ(num_nodes_walked, num_nodes, "`pos != 0` at end of PyTreeSpec::BroadcastToCommonSuffix() " @@ -529,7 +525,7 @@ std::unique_ptr PyTreeSpec::Transform(const std::optional(*this); } - const auto transform = + const auto transform_node = [this, &f_node, &f_leaf](const Node &node) -> std::unique_ptr { auto nodespec = GetOneLevel(node); @@ -540,10 +536,11 @@ std::unique_ptr PyTreeSpec::Transform(const std::optional(out)) [[unlikely]] { - std::ostringstream oss{}; - oss << "Expected the PyTreeSpec transform function returns a PyTreeSpec, got " - << PyRepr(out) << " (input: " << GetOneLevel(node)->ToString() << ")."; - throw py::type_error(oss.str()); + throw py::type_error( + std::format("Expected the PyTreeSpec transform function returns a PyTreeSpec, " + "got {} (input: {}).", + out, + GetOneLevel(node)->ToString())); } return std::make_unique(thread_safe_cast(out)); }; @@ -554,44 +551,41 @@ std::unique_ptr PyTreeSpec::Transform(const std::optional>(4); for (const Node &node : m_traversal) { - auto transformed = transform(node); + auto transformed = transform_node(node); if (transformed->m_none_is_leaf != m_none_is_leaf) [[unlikely]] { - std::ostringstream oss{}; - oss << "Expected the PyTreeSpec transform function returns " - "a PyTreeSpec with the same value of " - << (m_none_is_leaf ? "`none_is_leaf=True`" : "`none_is_leaf=False`") - << " as the input, got " << transformed->ToString() - << " (input: " << GetOneLevel(node)->ToString() << ")."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Expected the PyTreeSpec transform function returns a PyTreeSpec " + "with the same value of {} as the input, got {} (input: {}).", + m_none_is_leaf ? "`none_is_leaf=True`" : "`none_is_leaf=False`", + transformed->ToString(), + GetOneLevel(node)->ToString())); } if (!transformed->m_namespace.empty()) [[unlikely]] { if (common_registry_namespace.empty()) [[likely]] { common_registry_namespace = transformed->m_namespace; } else if (transformed->m_namespace != common_registry_namespace) [[unlikely]] { - std::ostringstream oss{}; - oss << "Expected the PyTreeSpec transform function returns " - "a PyTreeSpec with namespace " - << PyRepr(common_registry_namespace) << ", got " - << PyRepr(transformed->m_namespace) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Expected the PyTreeSpec transform function returns a PyTreeSpec " + "with namespace {}, got {}.", + PyRepr(common_registry_namespace), + PyRepr(transformed->m_namespace))); } } if (node.kind != PyTreeKind::Leaf) [[likely]] { if (transformed->GetNumLeaves() != node.arity) [[unlikely]] { - std::ostringstream oss{}; - oss << "Expected the PyTreeSpec transform function returns " - "a PyTreeSpec with the same number of arity as the input (" - << node.arity << "), got " << transformed->ToString() - << " (input: " << GetOneLevel(node)->ToString() << ")."; - throw py::value_error(oss.str()); + throw py::value_error(std::format( + "Expected the PyTreeSpec transform function returns a PyTreeSpec " + "with the same number of arity as the input ({}), got {} (input: {}).", + node.arity, + transformed->ToString(), + GetOneLevel(node)->ToString())); } if (transformed->GetNumNodes() != node.arity + 1) [[unlikely]] { - std::ostringstream oss{}; - oss << "Expected the PyTreeSpec transform function returns a one-level PyTreeSpec " - "as the input, got " - << transformed->ToString() << " (input: " << GetOneLevel(node)->ToString() - << ")."; - throw py::value_error(oss.str()); + throw py::value_error(std::format( + "Expected the PyTreeSpec transform function returns a one-level PyTreeSpec " + "as the input, got {} (input: {}).", + transformed->ToString(), + GetOneLevel(node)->ToString())); } auto &subroot = treespec->m_traversal.emplace_back(transformed->m_traversal.back()); EXPECT_GE(py::ssize_t_cast(pending_num_leaves_nodes.size()), @@ -607,9 +601,7 @@ std::unique_ptr PyTreeSpec::Transform(const std::optionalm_traversal.cbegin(), - transformed->m_traversal.cend(), - std::back_inserter(treespec->m_traversal)); + std::ranges::copy(transformed->m_traversal, std::back_inserter(treespec->m_traversal)); const ssize_t num_leaves = transformed->GetNumLeaves(); const ssize_t num_nodes = transformed->GetNumNodes(); num_extra_leaves += num_leaves - 1; @@ -644,11 +636,11 @@ std::unique_ptr PyTreeSpec::Transform(const std::optionalFindStaleCustomType(common_registry_namespace)) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTreeSpecs cannot be transformed: custom PyTree type " << PyRepr(*stale_type) - << " no longer resolves to its original registration in namespace " - << PyRepr(common_registry_namespace) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("PyTreeSpecs cannot be transformed: custom PyTree type {} no longer " + "resolves to its original registration in namespace {}.", + *stale_type, + PyRepr(common_registry_namespace))); } } @@ -667,10 +659,10 @@ std::unique_ptr PyTreeSpec::Compose(const PyTreeSpec &inner) const { } if (!m_namespace.empty() && !inner.m_namespace.empty() && m_namespace != inner.m_namespace) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTreeSpecs must have the same namespace, got " << PyRepr(m_namespace) << " vs. " - << PyRepr(inner.m_namespace) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("PyTreeSpecs must have the same namespace, got {} vs. {}.", + PyRepr(m_namespace), + PyRepr(inner.m_namespace))); } const std::string &target_namespace = m_namespace.empty() ? inner.m_namespace : m_namespace; @@ -694,11 +686,11 @@ std::unique_ptr PyTreeSpec::Compose(const PyTreeSpec &inner) const { stale_type = inner.FindStaleCustomType(target_namespace); } if (stale_type) [[unlikely]] { - std::ostringstream oss{}; - oss << "PyTreeSpecs cannot be merged: custom PyTree type " << PyRepr(*stale_type) - << " no longer resolves to its original registration in namespace " - << PyRepr(target_namespace) << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("PyTreeSpecs cannot be merged: custom PyTree type {} no longer " + "resolves to its original registration in namespace {}.", + *stale_type, + PyRepr(target_namespace))); } } else [[unlikely]] { EXPECT_TRUE(m_namespace.empty(), "Namespace mismatch."); @@ -711,9 +703,7 @@ std::unique_ptr PyTreeSpec::Compose(const PyTreeSpec &inner) const { const ssize_t num_inner_nodes = inner.GetNumNodes(); for (const Node &node : m_traversal) { if (node.kind == PyTreeKind::Leaf) [[likely]] { - std::copy(inner.m_traversal.cbegin(), - inner.m_traversal.cend(), - std::back_inserter(treespec->m_traversal)); + std::ranges::copy(inner.m_traversal, std::back_inserter(treespec->m_traversal)); } else [[unlikely]] { Node new_node{node}; new_node.num_leaves = node.num_leaves * num_inner_leaves; @@ -825,14 +815,14 @@ std::vector PyTreeSpec::Paths() const { } auto stack = reserved_vector(4); const ssize_t num_nodes_walked = PathsImpl(paths, stack, num_nodes - 1, 0); - std::reverse(paths.begin(), paths.end()); + std::ranges::reverse(paths); EXPECT_EQ(num_nodes_walked, num_nodes, "`pos != 0` at end of PyTreeSpec::Paths()."); EXPECT_EQ(py::ssize_t_cast(paths.size()), num_leaves, "PyTreeSpec::Paths() mismatched leaves."); return paths; } -template -ssize_t PyTreeSpec::AccessorsImpl(Span &accessors, // NOLINT[misc-no-recursion] +template +ssize_t PyTreeSpec::AccessorsImpl(AccessorVector &accessors, // NOLINT[misc-no-recursion] Stack &stack, const ssize_t &pos, const ssize_t &depth) const { @@ -937,7 +927,7 @@ std::vector PyTreeSpec::Accessors() const { const ssize_t num_nodes = GetNumNodes(); auto stack = reserved_vector(4); const ssize_t num_nodes_walked = AccessorsImpl(accessors, stack, num_nodes - 1, 0); - std::reverse(accessors.begin(), accessors.end()); + std::ranges::reverse(accessors); EXPECT_EQ(num_nodes_walked, num_nodes, "`pos != 0` at end of PyTreeSpec::Accessors()."); EXPECT_EQ(py::ssize_t_cast(accessors.size()), num_leaves, @@ -1042,9 +1032,9 @@ std::vector> PyTreeSpec::Children() const { children[i]->m_namespace = m_namespace; const Node &node = m_traversal.at(pos - 1); EXPECT_GE(pos, node.num_nodes, "PyTreeSpec::Children() walked off start of array."); - std::copy(m_traversal.cbegin() + pos - node.num_nodes, - m_traversal.cbegin() + pos, - std::back_inserter(children[i]->m_traversal)); + std::ranges::copy(m_traversal.cbegin() + (pos - node.num_nodes), + m_traversal.cbegin() + pos, + std::back_inserter(children[i]->m_traversal)); children[i]->m_traversal.shrink_to_fit(); PYTREESPEC_SANITY_CHECK(*children[i]); pos -= node.num_nodes; @@ -1076,9 +1066,9 @@ std::unique_ptr PyTreeSpec::Child(ssize_t index) const { child->m_namespace = m_namespace; const Node &node = m_traversal.at(pos - 1); EXPECT_GE(pos, node.num_nodes, "PyTreeSpec::Child() walked off start of array."); - std::copy(m_traversal.cbegin() + pos - node.num_nodes, - m_traversal.cbegin() + pos, - std::back_inserter(child->m_traversal)); + std::ranges::copy(m_traversal.cbegin() + (pos - node.num_nodes), + m_traversal.cbegin() + pos, + std::back_inserter(child->m_traversal)); child->m_traversal.shrink_to_fit(); PYTREESPEC_SANITY_CHECK(*child); return child; diff --git a/src/treespec/unflatten.cpp b/src/treespec/unflatten.cpp index 137d1b14..8b25854f 100644 --- a/src/treespec/unflatten.cpp +++ b/src/treespec/unflatten.cpp @@ -15,7 +15,8 @@ limitations under the License. ================================================================================ */ -#include // std::ostringstream +#include // std::format +#include // std::span #include // std::move #include "optree/optree.h" @@ -35,10 +36,10 @@ py::object PyTreeSpec::UnflattenImpl(const Span &leaves) const { switch (node.kind) { case PyTreeKind::Leaf: { if (it == leaves.end()) [[unlikely]] { - std::ostringstream oss{}; - oss << "Too few leaves for PyTreeSpec; expected: " << GetNumLeaves() - << ", got: " << num_leaves << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Too few leaves for PyTreeSpec; expected: {}, got: {}.", + GetNumLeaves(), + num_leaves)); } agenda.emplace_back(py::reinterpret_borrow(*it)); ++it; @@ -57,9 +58,10 @@ py::object PyTreeSpec::UnflattenImpl(const Span &leaves) const { case PyTreeKind::StructSequence: case PyTreeKind::Custom: { const ssize_t size = py::ssize_t_cast(agenda.size()); - py::object out = MakeNode(node, - node.arity > 0 ? &agenda[size - node.arity] : nullptr, - node.arity); + py::object out = + MakeNode(node, + node.arity > 0 ? std::span(&agenda[size - node.arity], node.arity) + : std::span{}); agenda.resize(size - node.arity); agenda.emplace_back(std::move(out)); break; @@ -71,9 +73,8 @@ py::object PyTreeSpec::UnflattenImpl(const Span &leaves) const { } } if (it != leaves.end()) [[unlikely]] { - std::ostringstream oss{}; - oss << "Too many leaves for PyTreeSpec; expected: " << GetNumLeaves() << "."; - throw py::value_error(oss.str()); + throw py::value_error( + std::format("Too many leaves for PyTreeSpec; expected: {}.", GetNumLeaves())); } EXPECT_EQ(agenda.size(), 1U, "PyTreeSpec traversal did not yield a singleton."); return agenda.back(); diff --git a/tests/test_ops.py b/tests/test_ops.py index 08ddcbc9..f9ba34d4 100644 --- a/tests/test_ops.py +++ b/tests/test_ops.py @@ -37,6 +37,7 @@ TREES, Counter, CustomTuple, + EmptyTuple, FlatCache, MyAnotherDict, Py_DEBUG, @@ -545,6 +546,32 @@ def f_leaf(leaf): ) +@pytest.mark.parametrize( + 'tree', + [ + (), + [], + {}, + OrderedDict(), + defaultdict(int), + deque(), + EmptyTuple(), + {'a': (), 'b': [[], deque()], 'c': {'d': OrderedDict()}, 'e': 1}, + ], + ids=str, +) +def test_unflatten_and_walk_arity_zero_nodes(tree): + # `MakeNode` takes its children as a `std::span`, which the callers build as an empty span for a + # childless node instead of pointing one past the end of the agenda. Round-trip every arity-0 + # container, including one nested among non-empty siblings so empty and non-empty spans alternate + # within a single agenda. + treespec = optree.tree_structure(tree) + leaves = optree.tree_leaves(tree) + assert optree.tree_unflatten(treespec, leaves) == tree + assert treespec.walk(leaves) == tree + assert treespec.traverse(leaves) == tree + + def test_flatten_up_to(): treespec = optree.tree_structure([(1, 2), None, CustomTuple(foo=3, bar=7)]) tree = [({'foo': 7}, (3, 4)), None, CustomTuple(foo=(11, 9), bar=None)] diff --git a/tests/test_treespec.py b/tests/test_treespec.py index f068edf3..4a614d91 100644 --- a/tests/test_treespec.py +++ b/tests/test_treespec.py @@ -311,10 +311,63 @@ def test_treespec_namedtuple_repr_with_divergent_fields_raises_value_error(): assert str(treespec) == 'PyTreeSpec(Point(x=*, y=*))' Point._fields = ('x', 'y', 'z') # diverge: 3 fields vs the treespec's arity of 2 - with pytest.raises(ValueError, match=r'does not match the arity'): + # Pin the interpolated values, not just the phrase: the message is assembled by `std::format`, + # so a mis-ordered placeholder would still match a substring pattern. + with pytest.raises( + ValueError, + match=re.escape( + f'Number of fields (3) of namedtuple type {Point!r} does not match the arity (2) ' + f'of the treespec node.', + ), + ): repr(treespec) +def test_treespec_setstate_malformed_state_message_format(): + # `FromPicklable` reports every structural defect through one `Malformed pickled PyTreeSpec: {}.` + # template. `test_treespec_setstate_rejects_malformed_state` covers which states are rejected; + # this pins the rendered text, so the reason is actually interpolated and the period is kept. + obj = optree.PyTreeSpec.__new__(optree.PyTreeSpec) + with pytest.raises( + RuntimeError, + match=re.escape('Malformed pickled PyTreeSpec: the state is not a 3-tuple.'), + ): + obj.__setstate__((1, 2)) + + +def test_treespec_from_collection_rejects_stale_custom_registration(): + # Building a collection promotes the children's namespace onto the result. If the custom type no + # longer resolves to the registration the child node holds, the result would silently rebind it. + class Stale: + def __init__(self, *children): + self.children = list(children) + + def register(): + optree.register_pytree_node( + Stale, + lambda s: (s.children, None), + lambda _, children: Stale(*children), + namespace='stale', + ) + + register() + try: + treespec = optree.tree_structure(Stale(1, 2), namespace='stale') + optree.unregister_pytree_node(Stale, namespace='stale') + register() # same type and namespace, but a different registration object + + with pytest.raises( + ValueError, + match=re.escape( + f'PyTreeSpecs cannot be composed into a collection: custom PyTree type {Stale!r} ' + f"no longer resolves to its original registration in namespace 'stale'.", + ), + ): + optree.treespec_tuple((treespec,), namespace='stale') + finally: + optree.unregister_pytree_node(Stale, namespace='stale') + + def test_treespec_setstate_rejects_structseq_field_arity_mismatch(): # A PyStructSequence type's sequence-field count is fixed in C, so a node's arity must equal it # (unlike a namedtuple, whose `_fields` can be mutated after the fact). `FromPicklable` (via