From 8f41436b149bbe01b83ef3e8fc951e64ee5efea5 Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Mon, 23 Jan 2023 14:39:42 +0800 Subject: [PATCH 1/2] feat: add `clang-tidy` integration --- .clang-tidy | 26 ++++++++ .github/PULL_REQUEST_TEMPLATE.md | 2 +- .github/workflows/lint.yml | 4 ++ .gitignore | 1 + CONTRIBUTING.md | 2 +- CPPLINT.cfg | 3 + Makefile | 22 ++++++- include/registry.h | 2 +- include/treespec.h | 52 ++++++++-------- include/utils.h | 17 +++--- src/optree.cpp | 3 +- src/registry.cpp | 12 ++-- src/treespec/flatten.cpp | 102 +++++++++++++++++-------------- src/treespec/traversal.cpp | 6 +- src/treespec/treespec.cpp | 46 ++++++++------ src/treespec/unflatten.cpp | 6 +- 16 files changed, 188 insertions(+), 118 deletions(-) create mode 100644 .clang-tidy diff --git a/.clang-tidy b/.clang-tidy new file mode 100644 index 00000000..6649f7ca --- /dev/null +++ b/.clang-tidy @@ -0,0 +1,26 @@ +--- +# NOTE: there must be no spaces before the '-', so put the comma last. +InheritParentConfig: true +Checks: ' +abseil-*, +bugprone-*, +-bugprone-easily-swappable-parameters, +clang-analyzer-*, +cppcoreguidelines-*, +-cppcoreguidelines-avoid-non-const-global-variables, +-cppcoreguidelines-macro-usage, +-cppcoreguidelines-pro-type-reinterpret-cast, +hicpp-exception-baseclass, +hicpp-avoid-goto, +modernize-*, +-modernize-use-trailing-return-type, +performance-*, +readability-*, +-readability-identifier-length, +-readability-convert-member-functions-to-static, +misc-*, +-misc-const-correctness, +' +HeaderFilterRegex: '^include/.*$' +WarningsAsErrors: '*' +FormatStyle: file diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 042108a7..4d89a5a4 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -30,7 +30,7 @@ What types of changes does your code introduce? Put an `x` in all the boxes that Go over all the following points, and put an `x` in all the boxes that apply. If you are unsure about any of these, don't hesitate to ask. We are here to help! -- [ ] I have read the [CONTRIBUTION](https://optree.readthedocs.io/en/latest/developer/contributing.html) guide. (**required**) +- [ ] I have read the [CONTRIBUTION](https://github.com/metaopt/optree/blob/HEAD/CONTRIBUTING.md) guide. (**required**) - [ ] My change requires a change to the documentation. - [ ] I have updated the tests accordingly. (*required for a bug fix or a new feature*) - [ ] I have updated the documentation accordingly. diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 448deab7..ea68578b 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -67,6 +67,10 @@ jobs: run: | make clang-format + - name: clang-tidy + run: | + make clang-tidy + - name: addlicense run: | make addlicense diff --git a/.gitignore b/.gitignore index 6817b109..7fa16a38 100644 --- a/.gitignore +++ b/.gitignore @@ -348,6 +348,7 @@ DerivedData/ # *.ipr # CMake +cmake-build/ cmake-build-*/ # Mongo Explorer plugin diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 821d93b2..89d9307e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -47,7 +47,7 @@ We use several tools to secure code quality, including: - PEP8 code style: `black`, `isort`, `pylint`, `flake8` - Type hint check: `mypy` -- C++ Google-style: `cpplint`, `clang-format` +- C++ code style: `cpplint`, `clang-format`, `clang-tidy` - License: `addlicense` - Documentation: `pydocstyle`, `doc8` diff --git a/CPPLINT.cfg b/CPPLINT.cfg index 41265bb6..dd346401 100644 --- a/CPPLINT.cfg +++ b/CPPLINT.cfg @@ -1 +1,4 @@ linelength=100 +filter=-readability/nolint +filter=-readability/braces +filter=-whitespace/newline diff --git a/Makefile b/Makefile index e86ccc56..1cc15dbe 100644 --- a/Makefile +++ b/Makefile @@ -59,7 +59,7 @@ pre-commit-install: $(PYTHON) -m pre_commit install --install-hooks docs-install: - $(call check_pip_install,pydocstyle) + $(call check_pip_install_extra,pydocstyle,pydocstyle[toml]) $(call check_pip_install_extra,doc8,"doc8<1.0.0a0") if ! $(PYTHON) -c "import sys; exit(sys.version_info < (3, 8))"; then \ $(PYTHON) -m pip uninstall --yes importlib-metadata; \ @@ -78,6 +78,9 @@ pytest-install: $(call check_pip_install,pytest-cov) $(call check_pip_install,pytest-xdist) +cmake-install: + command -v cmake || $(call check_pip_install,cmake) + cpplint-install: $(call check_pip_install,cpplint) @@ -126,12 +129,27 @@ pre-commit: pre-commit-install # C++ linters +cmake-configure: cmake-install + cmake -S . -B cmake-build-debug \ + -DCMAKE_BUILD_TYPE=Debug \ + -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \ + -DPYTHON_EXECUTABLE="$(PYTHON)" \ + -DOPTREE_CXX_WERROR="$(OPTREE_CXX_WERROR)" + +cmake-build: cmake-configure + cmake --build cmake-build-debug --parallel + +cmake: cmake-build + cpplint: cpplint-install $(PYTHON) -m cpplint $(CXX_FILES) clang-format: clang-format-install $(CLANG_FORMAT) --style=file -i $(CXX_FILES) -n --Werror +clang-tidy: clang-tidy-install cmake-configure + clang-tidy -p=cmake-build-debug $(CXX_FILES) + # Documentation addlicense: addlicense-install @@ -153,7 +171,7 @@ clean-docs: # Utility functions -lint: flake8 py-format mypy pylint clang-format cpplint addlicense docstyle spelling +lint: flake8 py-format mypy pylint clang-format clang-tidy cpplint addlicense docstyle spelling format: py-format-install clang-format-install addlicense-install $(PYTHON) -m isort --project $(PROJECT_NAME) $(PYTHON_FILES) diff --git a/include/registry.h b/include/registry.h index a6ccb543..2215e9b9 100644 --- a/include/registry.h +++ b/include/registry.h @@ -1,5 +1,5 @@ /* -Copyright 2022 MetaOPT Team. All Rights Reserved. +Copyright 2022-2023 MetaOPT Team. All Rights Reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/include/treespec.h b/include/treespec.h index aa2fa87d..56430d73 100644 --- a/include/treespec.h +++ b/include/treespec.h @@ -66,7 +66,7 @@ class PyTreeSpec { // Recursive helper used to implement Flatten(). bool FlattenInto(const py::handle &handle, - std::vector &leaves, // NOLINT + std::vector &leaves, // NOLINT[runtime/references] const std::optional &leaf_predicate, const bool &none_is_leaf, const std::string ®istry_namespace); @@ -82,8 +82,8 @@ class PyTreeSpec { // Recursive helper used to implement FlattenWithPath(). bool FlattenIntoWithPath(const py::handle &handle, - std::vector &leaves, // NOLINT - std::vector &paths, // NOLINT + std::vector &leaves, // NOLINT[runtime/references] + std::vector &paths, // NOLINT[runtime/references] const std::optional &leaf_predicate, const bool &none_is_leaf, const std::string ®istry_namespace); @@ -91,7 +91,7 @@ class PyTreeSpec { // Flattens a PyTree up to this PyTreeSpec. 'this' must be a tree prefix of the tree-structure // of 'x'. For example, if we flatten a value [(1, (2, 3)), {"foo": 4}] with a PyTreeSpec [(*, // *), *], the result is the list of leaves [1, (2, 3), {"foo": 4}]. - py::list FlattenUpTo(const py::handle &full_tree) const; + [[nodiscard]] py::list FlattenUpTo(const py::handle &full_tree) const; // Tests whether the given list is a flat list of leaves. static bool AllLeaves(const py::iterable &iterable, @@ -99,10 +99,10 @@ class PyTreeSpec { const std::string ®istry_namespace = ""); // Returns an unflattened PyTree given an iterable of leaves and a PyTreeSpec. - py::object Unflatten(const py::iterable &leaves) const; + [[nodiscard]] py::object Unflatten(const py::iterable &leaves) const; // Composes two PyTreeSpecs, replacing the leaves of this tree with copies of `inner`. - std::unique_ptr Compose(const PyTreeSpec &inner_treespec) const; + [[nodiscard]] std::unique_ptr Compose(const PyTreeSpec &inner_treespec) const; // Makes a Tuple PyTreeSpec out of a vector of PyTreeSpecs. static std::unique_ptr Tuple(const std::vector &treespecs, @@ -114,30 +114,30 @@ class PyTreeSpec { // Makes a PyTreeSpec representing a `None` node. static std::unique_ptr None(const bool &none_is_leaf); - std::vector> Children() const; + [[nodiscard]] std::vector> Children() const; // Maps a function over a PyTree structure, applying f_leaf to each leaf, and // f_node(children, node_data) to each container node. - py::object Walk(const py::function &f_node, - const py::handle &f_leaf, - const py::iterable &leaves) const; + [[nodiscard]] py::object Walk(const py::function &f_node, + const py::handle &f_leaf, + const py::iterable &leaves) const; - ssize_t num_leaves() const; + [[nodiscard]] ssize_t num_leaves() const; - ssize_t num_nodes() const; + [[nodiscard]] ssize_t num_nodes() const; - ssize_t num_children() const; + [[nodiscard]] ssize_t num_children() const; - bool get_none_is_leaf() const; + [[nodiscard]] bool get_none_is_leaf() const; - std::string get_namespace() const; + [[nodiscard]] std::string get_namespace() const; bool operator==(const PyTreeSpec &other) const; bool operator!=(const PyTreeSpec &other) const; template friend H AbslHashValue(H h, const Node &n) { - ssize_t data_hash{0}; + ssize_t data_hash = 0; switch (n.kind) { case PyTreeKind::Custom: // We don't hash node_data custom node types since they may not hashable. @@ -154,7 +154,7 @@ class PyTreeSpec { case PyTreeKind::OrderedDict: case PyTreeKind::DefaultDict: { py::list keys; - if (n.kind == PyTreeKind::DefaultDict) [[unlikely]] { // NOLINT + if (n.kind == PyTreeKind::DefaultDict) [[unlikely]] { EXPECT_EQ( GET_SIZE(n.node_data), 2, "Number of auxiliary data mismatch."); py::object default_factory = GET_ITEM_BORROW(n.node_data, 0); @@ -164,7 +164,7 @@ class PyTreeSpec { n.arity, "Number of keys and entries does not match."); data_hash = py::hash(default_factory); - } else [[likely]] { // NOLINT + } else [[likely]] { EXPECT_EQ(GET_SIZE(n.node_data), n.arity, "Number of keys and entries does not match."); @@ -190,11 +190,11 @@ class PyTreeSpec { return h; } - std::string ToString() const; + [[nodiscard]] std::string ToString() const; // Transforms the PyTreeSpec into a picklable object. // Used to implement `PyTreeSpec.__getstate__`. - py::object ToPicklable() const; + [[nodiscard]] py::object ToPicklable() const; // Transforms the object returned by `ToPicklable()` back to PyTreeSpec. // Used to implement `PyTreeSpec.__setstate__`. @@ -240,7 +240,7 @@ class PyTreeSpec { // Whether to treat `None` as a leaf. If false, `None` is a non-leaf node with arity 0. bool m_none_is_leaf = false; - // The registry namespace used to resolve the custom pytree node types + // The registry namespace used to resolve the custom pytree node types. std::string m_namespace; // Helper that manufactures an instance of a node given its children. @@ -254,21 +254,21 @@ class PyTreeSpec { template bool FlattenIntoImpl(const py::handle &handle, - Span &leaves, // NOLINT + Span &leaves, // NOLINT[runtime/references] const ssize_t &depth, const std::optional &leaf_predicate, const std::string ®istry_namespace); template bool FlattenIntoWithPathImpl(const py::handle &handle, - Span &leaves, // NOLINT - Span &paths, // NOLINT - Stack &stack, // NOLINT + Span &leaves, // NOLINT[runtime/references] + Span &paths, // NOLINT[runtime/references] + Stack &stack, // NOLINT[runtime/references] const ssize_t &depth, const std::optional &leaf_predicate, const std::string ®istry_namespace); - py::list FlattenUpToImpl(const py::handle &full_tree) const; + [[nodiscard]] py::list FlattenUpToImpl(const py::handle &full_tree) const; template static bool AllLeavesImpl(const py::iterable &iterable, const std::string ®istry_namespace); diff --git a/include/utils.h b/include/utils.h index af8679ee..2f2f9eeb 100644 --- a/include/utils.h +++ b/include/utils.h @@ -1,5 +1,5 @@ /* -Copyright 2022 MetaOPT Team. All Rights Reserved. +Copyright 2022-2023 MetaOPT Team. All Rights Reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -46,25 +46,25 @@ using ssize_t = py::ssize_t; inline const py::module& ImportCollections() { // NOTE: Use raw pointers to leak the memory intentionally to avoid py::object deallocation and - // garbage collection + // garbage collection. static const py::module* ptr = new py::module{py::module::import("collections")}; return *ptr; } inline const py::object& ImportOrderedDict() { // NOTE: Use raw pointers to leak the memory intentionally to avoid py::object deallocation and - // garbage collection + // garbage collection. static const py::object* ptr = new py::object{py::getattr(PyCollectionsModule, "OrderedDict")}; return *ptr; } inline const py::object& ImportDefaultDict() { // NOTE: Use raw pointers to leak the memory intentionally to avoid py::object deallocation and - // garbage collection + // garbage collection. static const py::object* ptr = new py::object{py::getattr(PyCollectionsModule, "defaultdict")}; return *ptr; } inline const py::object& ImportDeque() { // NOTE: Use raw pointers to leak the memory intentionally to avoid py::object deallocation and - // garbage collection + // garbage collection. static const py::object* ptr = new py::object{py::getattr(PyCollectionsModule, "deque")}; return *ptr; } @@ -85,11 +85,12 @@ inline py::list SortedDictKeys(const py::dict& dict) { try { // Sort directly if possible. + // NOLINTNEXTLINE[readability-implicit-bool-conversion] if (PyList_Sort(keys.ptr())) [[unlikely]] { throw py::error_already_set(); } } catch (py::error_already_set& ex1) { - if (ex1.matches(PyExc_TypeError)) [[likely]] { // NOLINT + if (ex1.matches(PyExc_TypeError)) [[likely]] { // Found incomparable keys (e.g. `int` vs. `str`, or user-defined types). try { // Sort with `(f'{o.__class__.__module__}.{o.__class__.__qualname__}', o)` @@ -107,11 +108,11 @@ inline py::list SortedDictKeys(const py::dict& dict) { // Found incomparable user-defined key types. // The keys remain in the insertion order. PyErr_Clear(); - } else [[unlikely]] { // NOLINT + } else [[unlikely]] { throw; } } - } else [[unlikely]] { // NOLINT + } else [[unlikely]] { throw; } } diff --git a/src/optree.cpp b/src/optree.cpp index 845acccd..477dbee8 100644 --- a/src/optree.cpp +++ b/src/optree.cpp @@ -27,7 +27,7 @@ limitations under the License. namespace optree { -void BuildModule(py::module& mod) { // NOLINT +void BuildModule(py::module& mod) { // NOLINT[runtime/references] mod.doc() = "Optimized PyTree Utilities."; py::register_local_exception(mod, "InternalError", PyExc_RuntimeError); mod.attr("MAX_RECURSION_DEPTH") = MAX_RECURSION_DEPTH; @@ -144,4 +144,5 @@ void BuildModule(py::module& mod) { // NOLINT } // namespace optree +// NOLINTNEXTLINE[cppcoreguidelines-pro-bounds-pointer-arithmetic,cppcoreguidelines-pro-type-vararg] PYBIND11_MODULE(_C, mod) { optree::BuildModule(mod); } diff --git a/src/registry.cpp b/src/registry.cpp index fb171710..18da5ebe 100644 --- a/src/registry.cpp +++ b/src/registry.cpp @@ -1,5 +1,5 @@ /* -Copyright 2022 MetaOPT Team. All Rights Reserved. +Copyright 2022-2023 MetaOPT Team. All Rights Reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -36,7 +36,9 @@ template absl::StrFormat("PyTree type %s is already registered in the global namespace.", py::repr(type))); }; - if (!NoneIsLeaf) add_builtin_type(Py_TYPE(Py_None), PyTreeKind::None); + if (!NoneIsLeaf) { + add_builtin_type(Py_TYPE(Py_None), PyTreeKind::None); + } add_builtin_type(&PyTuple_Type, PyTreeKind::Tuple); add_builtin_type(&PyList_Type, PyTreeKind::List); add_builtin_type(&PyDict_Type, PyTreeKind::Dict); @@ -67,12 +69,12 @@ template registration->type = py::reinterpret_borrow(cls); registration->to_iterable = py::reinterpret_borrow(to_iterable); registration->from_iterable = py::reinterpret_borrow(from_iterable); - if (registry_namespace.empty()) [[unlikely]] { // NOLINT + if (registry_namespace.empty()) [[unlikely]] { if (!registry->m_registrations.emplace(cls, std::move(registration)).second) [[unlikely]] { throw std::invalid_argument(absl::StrFormat( "PyTree type %s is already registered in the global namespace.", py::repr(cls))); } - } else [[likely]] { // NOLINT + } else [[likely]] { if (registry->m_registrations.find(cls) != registry->m_registrations.end()) [[unlikely]] { throw std::invalid_argument(absl::StrFormat( "PyTree type %s is already registered in the global namespace.", py::repr(cls))); @@ -109,7 +111,7 @@ template } if (registry_namespace.empty()) [[likely]] { return nullptr; - } else [[unlikely]] { // NOLINT + } else [[unlikely]] { auto named_it = registry->m_named_registrations.find(std::make_pair(registry_namespace, type)); return named_it != registry->m_named_registrations.end() ? named_it->second.get() : nullptr; diff --git a/src/treespec/flatten.cpp b/src/treespec/flatten.cpp index 67556a04..e7e07881 100644 --- a/src/treespec/flatten.cpp +++ b/src/treespec/flatten.cpp @@ -1,5 +1,5 @@ /* -Copyright 2022 MetaOPT Team. All Rights Reserved. +Copyright 2022-2023 MetaOPT Team. All Rights Reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -23,25 +23,27 @@ limitations under the License. namespace optree { template +// NOLINTNEXTLINE[readability-function-cognitive-complexity] bool PyTreeSpec::FlattenIntoImpl(const py::handle& handle, Span& leaves, const ssize_t& depth, const std::optional& leaf_predicate, const std::string& registry_namespace) { - if (depth > MAX_RECURSION_DEPTH) [[unlikely]] { // NOLINT + if (depth > MAX_RECURSION_DEPTH) [[unlikely]] { PyErr_SetString(PyExc_RecursionError, "maximum recursion depth exceeded during flattening the tree"); throw py::error_already_set(); } - bool found_custom{false}; + bool found_custom = false; Node node; ssize_t start_num_nodes = py::ssize_t_cast(m_traversal.size()); - ssize_t start_num_leaves = leaves.size(); + ssize_t start_num_leaves = py::ssize_t_cast(leaves.size()); if (leaf_predicate && (*leaf_predicate)(handle).cast()) [[unlikely]] { leaves.emplace_back(py::reinterpret_borrow(handle)); - } else [[likely]] { // NOLINT + } else [[likely]] { node.kind = GetKind(handle, &node.custom, registry_namespace); + // NOLINTNEXTLINE[misc-no-recursion] auto recurse = [this, &found_custom, &leaf_predicate, ®istry_namespace, &leaves, &depth]( py::handle child) { found_custom |= FlattenIntoImpl( @@ -49,7 +51,9 @@ bool PyTreeSpec::FlattenIntoImpl(const py::handle& handle, }; switch (node.kind) { case PyTreeKind::None: - if (!NoneIsLeaf) break; + if (!NoneIsLeaf) { + break; + } [[fallthrough]]; case PyTreeKind::Leaf: leaves.emplace_back(py::reinterpret_borrow(handle)); @@ -74,11 +78,11 @@ bool PyTreeSpec::FlattenIntoImpl(const py::handle& handle, case PyTreeKind::Dict: case PyTreeKind::OrderedDict: case PyTreeKind::DefaultDict: { - py::dict dict = py::reinterpret_borrow(handle); + auto dict = py::reinterpret_borrow(handle); py::list keys; if (node.kind == PyTreeKind::OrderedDict) [[unlikely]] { keys = DictKeys(dict); - } else [[likely]] { // NOLINT + } else [[likely]] { keys = SortedDictKeys(dict); } for (const py::handle& key : keys) { @@ -88,14 +92,14 @@ bool PyTreeSpec::FlattenIntoImpl(const py::handle& handle, if (node.kind == PyTreeKind::DefaultDict) [[unlikely]] { node.node_data = py::make_tuple(py::getattr(handle, "default_factory"), std::move(keys)); - } else [[likely]] { // NOLINT + } else [[likely]] { node.node_data = std::move(keys); } break; } case PyTreeKind::NamedTuple: { - py::tuple tuple = py::reinterpret_borrow(handle); + auto tuple = py::reinterpret_borrow(handle); node.arity = GET_SIZE(tuple); node.node_data = py::reinterpret_borrow(tuple.get_type()); for (ssize_t i = 0; i < node.arity; ++i) { @@ -105,7 +109,7 @@ bool PyTreeSpec::FlattenIntoImpl(const py::handle& handle, } case PyTreeKind::Deque: { - py::list list = handle.cast(); + auto list = handle.cast(); node.arity = GET_SIZE(list); node.node_data = py::getattr(handle, "maxlen"); for (ssize_t i = 0; i < node.arity; ++i) { @@ -118,7 +122,7 @@ bool PyTreeSpec::FlattenIntoImpl(const py::handle& handle, found_custom = true; py::tuple out = py::cast(node.custom->to_iterable(handle)); const size_t num_out = GET_SIZE(out); - if (num_out != 2 && num_out != 3) [[unlikely]] { // NOLINT + if (num_out != 2 && num_out != 3) [[unlikely]] { throw std::runtime_error( "PyTree custom to_iterable function should return a pair or a triple."); } @@ -129,9 +133,9 @@ bool PyTreeSpec::FlattenIntoImpl(const py::handle& handle, ++node.arity; recurse(child); } - if (num_out == 3) [[likely]] { // NOLINT + if (num_out == 3) [[likely]] { py::object node_entries = GET_ITEM_BORROW(out, 2); - if (!node_entries.is_none()) [[likely]] { // NOLINT + if (!node_entries.is_none()) [[likely]] { node.node_entries = py::cast(std::move(node_entries)); const ssize_t num_entries = GET_SIZE(node.node_entries); if (num_entries != node.arity) [[unlikely]] { @@ -163,7 +167,7 @@ bool PyTreeSpec::FlattenInto(const py::handle& handle, const std::string& registry_namespace) { if (none_is_leaf) [[unlikely]] { return FlattenIntoImpl(handle, leaves, 0, leaf_predicate, registry_namespace); - } else [[likely]] { // NOLINT + } else [[likely]] { return FlattenIntoImpl(handle, leaves, 0, leaf_predicate, registry_namespace); } } @@ -184,6 +188,7 @@ bool PyTreeSpec::FlattenInto(const py::handle& handle, } template +// NOLINTNEXTLINE[readability-function-cognitive-complexity] bool PyTreeSpec::FlattenIntoWithPathImpl(const py::handle& handle, Span& leaves, Span& paths, @@ -191,20 +196,21 @@ bool PyTreeSpec::FlattenIntoWithPathImpl(const py::handle& handle, const ssize_t& depth, const std::optional& leaf_predicate, const std::string& registry_namespace) { - if (depth > MAX_RECURSION_DEPTH) [[unlikely]] { // NOLINT + if (depth > MAX_RECURSION_DEPTH) [[unlikely]] { PyErr_SetString(PyExc_RecursionError, "maximum recursion depth exceeded during flattening the tree"); throw py::error_already_set(); } - bool found_custom{false}; + bool found_custom = false; Node node; ssize_t start_num_nodes = py::ssize_t_cast(m_traversal.size()); - ssize_t start_num_leaves = leaves.size(); + ssize_t start_num_leaves = py::ssize_t_cast(leaves.size()); if (leaf_predicate && (*leaf_predicate)(handle).cast()) [[unlikely]] { leaves.emplace_back(py::reinterpret_borrow(handle)); - } else [[likely]] { // NOLINT + } else [[likely]] { node.kind = GetKind(handle, &node.custom, registry_namespace); + // NOLINTNEXTLINE[misc-no-recursion] auto recurse = [this, &found_custom, &leaf_predicate, @@ -220,10 +226,12 @@ bool PyTreeSpec::FlattenIntoWithPathImpl(const py::handle& handle, }; switch (node.kind) { case PyTreeKind::None: - if (!NoneIsLeaf) break; + if (!NoneIsLeaf) { + break; + } [[fallthrough]]; case PyTreeKind::Leaf: { - py::tuple path = py::tuple{depth}; + py::tuple path{depth}; for (ssize_t d = 0; d < depth; ++d) { SET_ITEM(path, d, stack[d]); } @@ -251,11 +259,11 @@ bool PyTreeSpec::FlattenIntoWithPathImpl(const py::handle& handle, case PyTreeKind::Dict: case PyTreeKind::OrderedDict: case PyTreeKind::DefaultDict: { - py::dict dict = py::reinterpret_borrow(handle); + auto dict = py::reinterpret_borrow(handle); py::list keys; if (node.kind == PyTreeKind::OrderedDict) [[unlikely]] { keys = DictKeys(dict); - } else [[likely]] { // NOLINT + } else [[likely]] { keys = SortedDictKeys(dict); } for (const py::handle& key : keys) { @@ -265,14 +273,14 @@ bool PyTreeSpec::FlattenIntoWithPathImpl(const py::handle& handle, if (node.kind == PyTreeKind::DefaultDict) [[unlikely]] { node.node_data = py::make_tuple(py::getattr(handle, "default_factory"), std::move(keys)); - } else [[likely]] { // NOLINT + } else [[likely]] { node.node_data = std::move(keys); } break; } case PyTreeKind::NamedTuple: { - py::tuple tuple = py::reinterpret_borrow(handle); + auto tuple = py::reinterpret_borrow(handle); node.arity = GET_SIZE(tuple); node.node_data = py::reinterpret_borrow(tuple.get_type()); for (ssize_t i = 0; i < node.arity; ++i) { @@ -282,7 +290,7 @@ bool PyTreeSpec::FlattenIntoWithPathImpl(const py::handle& handle, } case PyTreeKind::Deque: { - py::list list = handle.cast(); + auto list = handle.cast(); node.arity = GET_SIZE(list); node.node_data = py::getattr(handle, "maxlen"); for (ssize_t i = 0; i < node.arity; ++i) { @@ -302,17 +310,17 @@ bool PyTreeSpec::FlattenIntoWithPathImpl(const py::handle& handle, node.arity = 0; node.node_data = GET_ITEM_BORROW(out, 1); py::object node_entries; - if (num_out == 3) [[likely]] { // NOLINT + if (num_out == 3) [[likely]] { node_entries = GET_ITEM_BORROW(out, 2); - } else [[unlikely]] { // NOLINT + } else [[unlikely]] { node_entries = py::none(); } - if (node_entries.is_none()) [[unlikely]] { // NOLINT + if (node_entries.is_none()) [[unlikely]] { for (const py::handle& child : py::cast(GET_ITEM_BORROW(out, 0))) { recurse(child, py::int_(node.arity++)); } - } else [[likely]] { // NOLINT + } else [[likely]] { node.node_entries = py::cast(std::move(node_entries)); node.arity = GET_SIZE(node.node_entries); ssize_t num_children = 0; @@ -353,11 +361,11 @@ bool PyTreeSpec::FlattenIntoWithPath(const py::handle& handle, const std::optional& leaf_predicate, const bool& none_is_leaf, const std::string& registry_namespace) { - std::vector stack; + auto stack = std::vector{}; if (none_is_leaf) [[unlikely]] { return FlattenIntoWithPathImpl( handle, leaves, paths, stack, 0, leaf_predicate, registry_namespace); - } else [[likely]] { // NOLINT + } else [[likely]] { return FlattenIntoWithPathImpl( handle, leaves, paths, stack, 0, leaf_predicate, registry_namespace); } @@ -379,11 +387,11 @@ PyTreeSpec::FlattenWithPath(const py::handle& tree, return std::make_tuple(std::move(paths), std::move(leaves), std::move(treespec)); } +// NOLINTNEXTLINE[readability-function-cognitive-complexity] py::list PyTreeSpec::FlattenUpToImpl(const py::handle& full_tree) const { const ssize_t num_leaves = PyTreeSpec::num_leaves(); - std::vector agenda; - agenda.emplace_back(py::reinterpret_borrow(full_tree)); + auto agenda = std::vector{py::reinterpret_borrow(full_tree)}; auto it = m_traversal.rbegin(); py::list leaves{num_leaves}; @@ -410,7 +418,7 @@ py::list PyTreeSpec::FlattenUpToImpl(const py::handle& full_tree) const { case PyTreeKind::Tuple: { AssertExact(object); - py::tuple tuple = py::reinterpret_borrow(object); + auto tuple = py::reinterpret_borrow(object); if (GET_SIZE(tuple) != node.arity) [[unlikely]] { throw std::invalid_argument( absl::StrFormat("Tuple arity mismatch: %ld != %ld; tuple: %s.", @@ -426,7 +434,7 @@ py::list PyTreeSpec::FlattenUpToImpl(const py::handle& full_tree) const { case PyTreeKind::List: { AssertExact(object); - py::list list = py::reinterpret_borrow(object); + auto list = py::reinterpret_borrow(object); if (GET_SIZE(list) != node.arity) [[unlikely]] { throw std::invalid_argument( absl::StrFormat("List arity mismatch: %ld != %ld; list: %s.", @@ -444,14 +452,14 @@ py::list PyTreeSpec::FlattenUpToImpl(const py::handle& full_tree) const { case PyTreeKind::OrderedDict: { if (node.kind == PyTreeKind::OrderedDict) [[unlikely]] { AssertExactOrderedDict(object); - } else [[likely]] { // NOLINT + } else [[likely]] { AssertExact(object); } - py::dict dict = py::reinterpret_borrow(object); + auto dict = py::reinterpret_borrow(object); py::list keys; if (node.kind == PyTreeKind::OrderedDict) [[unlikely]] { keys = DictKeys(dict); - } else [[likely]] { // NOLINT + } else [[likely]] { keys = SortedDictKeys(dict); } if (keys.not_equal(node.node_data)) [[unlikely]] { @@ -468,7 +476,7 @@ py::list PyTreeSpec::FlattenUpToImpl(const py::handle& full_tree) const { case PyTreeKind::NamedTuple: { AssertExactNamedTuple(object); - py::tuple tuple = py::reinterpret_borrow(object); + auto tuple = py::reinterpret_borrow(object); if (GET_SIZE(tuple) != node.arity) [[unlikely]] { throw std::invalid_argument( absl::StrFormat("Named tuple arity mismatch: %ld != %ld; tuple: %s.", @@ -490,8 +498,8 @@ py::list PyTreeSpec::FlattenUpToImpl(const py::handle& full_tree) const { case PyTreeKind::DefaultDict: { AssertExactDefaultDict(object); - py::dict dict = py::reinterpret_borrow(object); - py::list keys = SortedDictKeys(dict); + auto dict = py::reinterpret_borrow(object); + const py::list keys = SortedDictKeys(dict); py::object default_factory = py::getattr(object, "default_factory"); py::object expected_default_factory = GET_ITEM_BORROW(node.node_data, 0); py::list expected_keys = GET_ITEM_BORROW(node.node_data, 1); @@ -515,7 +523,7 @@ py::list PyTreeSpec::FlattenUpToImpl(const py::handle& full_tree) const { case PyTreeKind::Deque: { AssertExactDeque(object); - py::list list = py::cast(object); + auto list = py::cast(object); if (GET_SIZE(list) != node.arity) [[unlikely]] { throw std::invalid_argument( absl::StrFormat("Deque arity mismatch: %ld != %ld; deque: %s.", @@ -530,11 +538,11 @@ py::list PyTreeSpec::FlattenUpToImpl(const py::handle& full_tree) const { } case PyTreeKind::Custom: { - const PyTreeTypeRegistry::Registration* registration; + const PyTreeTypeRegistry::Registration* registration = nullptr; if (m_none_is_leaf) [[unlikely]] { registration = PyTreeTypeRegistry::Lookup(object.get_type(), m_namespace); - } else [[likely]] { // NOLINT + } else [[likely]] { registration = PyTreeTypeRegistry::Lookup(object.get_type(), m_namespace); } @@ -591,7 +599,7 @@ py::list PyTreeSpec::FlattenUpTo(const py::handle& full_tree) const { template /*static*/ bool PyTreeSpec::AllLeavesImpl(const py::iterable& iterable, const std::string& registry_namespace) { - const PyTreeTypeRegistry::Registration* custom; + const PyTreeTypeRegistry::Registration* custom = nullptr; for (const py::handle& h : iterable) { if (GetKind(h, &custom, registry_namespace) != PyTreeKind::Leaf) [[unlikely]] { return false; @@ -605,7 +613,7 @@ template const std::string& registry_namespace) { if (none_is_leaf) [[unlikely]] { return AllLeavesImpl(iterable, registry_namespace); - } else [[likely]] { // NOLINT + } else [[likely]] { return AllLeavesImpl(iterable, registry_namespace); } } diff --git a/src/treespec/traversal.cpp b/src/treespec/traversal.cpp index ac1e2bc3..5647c930 100644 --- a/src/treespec/traversal.cpp +++ b/src/treespec/traversal.cpp @@ -1,5 +1,5 @@ /* -Copyright 2022 MetaOPT Team. All Rights Reserved. +Copyright 2022-2023 MetaOPT Team. All Rights Reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -25,7 +25,7 @@ namespace optree { py::object PyTreeSpec::Walk(const py::function& f_node, const py::handle& f_leaf, const py::iterable& leaves) const { - std::vector agenda; + auto agenda = std::vector{}; auto it = leaves.begin(); const bool f_leaf_identity = f_leaf.is_none(); for (const Node& node : m_traversal) { @@ -35,7 +35,7 @@ py::object PyTreeSpec::Walk(const py::function& f_node, throw std::invalid_argument("Too few leaves for PyTreeSpec."); } - py::object leaf = py::reinterpret_borrow(*it); + auto leaf = py::reinterpret_borrow(*it); agenda.emplace_back(f_leaf_identity ? std::move(leaf) : f_leaf(std::move(leaf))); ++it; break; diff --git a/src/treespec/treespec.cpp b/src/treespec/treespec.cpp index 191cf915..5a329513 100644 --- a/src/treespec/treespec.cpp +++ b/src/treespec/treespec.cpp @@ -48,7 +48,9 @@ bool PyTreeSpec::operator==(const PyTreeSpec& other) const { return false; } + // NOLINTNEXTLINE[readability-qualified-auto] auto b = other.m_traversal.begin(); + // NOLINTNEXTLINE[readability-qualified-auto] for (auto a = m_traversal.begin(); a != m_traversal.end(); ++a, ++b) { if (a->kind != b->kind || a->arity != b->arity || (a->node_data.ptr() == nullptr) != (b->node_data.ptr() == nullptr) || @@ -67,11 +69,11 @@ bool PyTreeSpec::operator==(const PyTreeSpec& other) const { bool PyTreeSpec::operator!=(const PyTreeSpec& other) const { return !(*this == other); } std::unique_ptr PyTreeSpec::Compose(const PyTreeSpec& inner_treespec) const { - if (m_none_is_leaf != inner_treespec.m_none_is_leaf) [[unlikely]] { // NOLINT + if (m_none_is_leaf != inner_treespec.m_none_is_leaf) [[unlikely]] { throw std::invalid_argument("PyTreeSpecs must have the same none_is_leaf value."); } if (!m_namespace.empty() && !inner_treespec.m_namespace.empty() && - m_namespace != inner_treespec.m_namespace) [[unlikely]] { // NOLINT + m_namespace != inner_treespec.m_namespace) [[unlikely]] { throw std::invalid_argument( absl::StrFormat("PyTreeSpecs must have the same namespace. Got %s vs. %s.", py::repr(py::str(m_namespace)), @@ -82,7 +84,7 @@ std::unique_ptr PyTreeSpec::Compose(const PyTreeSpec& inner_treespec treespec->m_none_is_leaf = m_none_is_leaf; if (inner_treespec.m_namespace.empty()) [[likely]] { treespec->m_namespace = m_namespace; - } else [[unlikely]] { // NOLINT + } else [[unlikely]] { treespec->m_namespace = inner_treespec.m_namespace; } @@ -93,7 +95,7 @@ std::unique_ptr PyTreeSpec::Compose(const PyTreeSpec& inner_treespec for (const Node& node : m_traversal) { if (node.kind == PyTreeKind::Leaf) [[likely]] { absl::c_copy(inner_treespec.m_traversal, std::back_inserter(treespec->m_traversal)); - } else [[unlikely]] { // NOLINT + } else [[unlikely]] { Node new_node{node}; new_node.num_leaves = node.num_leaves * num_inner_leaves; new_node.num_nodes = @@ -120,7 +122,7 @@ std::unique_ptr PyTreeSpec::Compose(const PyTreeSpec& inner_treespec throw std::invalid_argument(absl::StrFormat( "Expected TreeSpecs with `node_is_leaf=%s`.", (none_is_leaf ? "True" : "False"))); } - if (!treespec.m_namespace.empty()) [[unlikely]] { // NOLINT + if (!treespec.m_namespace.empty()) [[unlikely]] { if (registry_namespace.empty()) [[likely]] { registry_namespace = treespec.m_namespace; } else if (registry_namespace != treespec.m_namespace) [[unlikely]] { @@ -177,7 +179,7 @@ std::unique_ptr PyTreeSpec::Compose(const PyTreeSpec& inner_treespec } std::vector> PyTreeSpec::Children() const { - std::vector> children; + auto children = std::vector>{}; if (m_traversal.empty()) [[likely]] { return children; } @@ -235,7 +237,7 @@ std::vector> PyTreeSpec::Children() const { case PyTreeKind::Dict: { py::dict dict; - py::list keys = py::reinterpret_borrow(node.node_data); + auto keys = py::reinterpret_borrow(node.node_data); for (ssize_t i = 0; i < node.arity; ++i) { dict[GET_ITEM_HANDLE(keys, i)] = std::move(children[i]); } @@ -244,7 +246,7 @@ std::vector> PyTreeSpec::Children() const { case PyTreeKind::OrderedDict: { py::list items{node.arity}; - py::list keys = py::reinterpret_borrow(node.node_data); + auto keys = py::reinterpret_borrow(node.node_data); for (ssize_t i = 0; i < node.arity; ++i) { SET_ITEM( items, @@ -283,10 +285,10 @@ template const std::string& registry_namespace) { const PyTreeTypeRegistry::Registration* registration = PyTreeTypeRegistry::Lookup(handle.get_type(), registry_namespace); - if (registration) [[likely]] { // NOLINT + if (registration) [[likely]] { if (registration->kind == PyTreeKind::Custom) [[unlikely]] { *custom = registration; - } else [[likely]] { // NOLINT + } else [[likely]] { *custom = nullptr; } return registration->kind; @@ -305,8 +307,9 @@ template PyTreeKind PyTreeSpec::GetKind(const py::handle&, PyTreeTypeRegistry::Registration const**, const std::string&); +// NOLINTNEXTLINE[readability-function-cognitive-complexity] std::string PyTreeSpec::ToString() const { - std::vector agenda; + auto agenda = std::vector{}; for (const Node& node : m_traversal) { EXPECT_GE(py::ssize_t_cast(agenda.size()), node.arity, "Too few elements for container."); @@ -332,10 +335,10 @@ std::string PyTreeSpec::ToString() const { case PyTreeKind::List: case PyTreeKind::Deque: representation = absl::StrCat("[", children, "]"); - if (node.kind == PyTreeKind::Deque) [[unlikely]] { // NOLINT + if (node.kind == PyTreeKind::Deque) [[unlikely]] { if (node.node_data.is_none()) [[likely]] { representation = absl::StrCat("deque(", representation, ")"); - } else [[unlikely]] { // NOLINT + } else [[unlikely]] { representation = absl::StrFormat( "deque(%s, maxlen=%s)", representation, py::str(node.node_data)); } @@ -361,7 +364,7 @@ std::string PyTreeSpec::ToString() const { case PyTreeKind::NamedTuple: { py::object type = node.node_data; - py::tuple fields = py::reinterpret_borrow(py::getattr(type, "_fields")); + auto fields = py::reinterpret_borrow(py::getattr(type, "_fields")); EXPECT_EQ(GET_SIZE(fields), node.arity, "Number of fields and entries does not match."); @@ -404,7 +407,7 @@ std::string PyTreeSpec::ToString() const { EXPECT_EQ( GET_SIZE(node.node_data), 2, "Number of auxiliary data mismatch."); py::object default_factory = GET_ITEM_BORROW(node.node_data, 0); - py::list keys = + auto keys = py::reinterpret_borrow(GET_ITEM_BORROW(node.node_data, 1)); EXPECT_EQ(GET_SIZE(keys), node.arity, @@ -469,17 +472,19 @@ py::object PyTreeSpec::ToPicklable() const { return py::make_tuple(std::move(node_states), py::bool_(m_none_is_leaf), py::str(m_namespace)); } +// NOLINTBEGIN[cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers] +// NOLINTNEXTLINE[readability-function-cognitive-complexity] /*static*/ PyTreeSpec PyTreeSpec::FromPicklableImpl(const py::object& picklable) { - py::tuple state = py::reinterpret_steal(picklable); + auto state = py::reinterpret_steal(picklable); if (state.size() != 3) [[unlikely]] { throw std::runtime_error("Malformed pickled PyTreeSpec."); } - bool none_is_leaf; + bool none_is_leaf = false; std::string registry_namespace; PyTreeSpec treespec; treespec.m_none_is_leaf = none_is_leaf = state[1].cast(); treespec.m_namespace = registry_namespace = state[2].cast(); - py::tuple node_states = py::reinterpret_borrow(state[0]); + auto node_states = py::reinterpret_borrow(state[0]); for (const auto& item : node_states) { auto t = item.cast(); if (t.size() != 7) [[unlikely]] { @@ -522,11 +527,11 @@ py::object PyTreeSpec::ToPicklable() const { } if (t[4].is_none()) [[unlikely]] { node.custom = nullptr; - } else [[likely]] { // NOLINT + } else [[likely]] { if (none_is_leaf) [[unlikely]] { node.custom = PyTreeTypeRegistry::Lookup(t[4], registry_namespace); - } else [[likely]] { // NOLINT + } else [[likely]] { node.custom = PyTreeTypeRegistry::Lookup(t[4], registry_namespace); } @@ -544,6 +549,7 @@ py::object PyTreeSpec::ToPicklable() const { } return treespec; } +// NOLINTEND[cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers] /*static*/ PyTreeSpec PyTreeSpec::FromPicklable(const py::object& picklable) { return FromPicklableImpl(picklable); diff --git a/src/treespec/unflatten.cpp b/src/treespec/unflatten.cpp index b7bd0754..945adfc8 100644 --- a/src/treespec/unflatten.cpp +++ b/src/treespec/unflatten.cpp @@ -1,5 +1,5 @@ /* -Copyright 2022 MetaOPT Team. All Rights Reserved. +Copyright 2022-2023 MetaOPT Team. All Rights Reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -24,7 +24,7 @@ namespace optree { template py::object PyTreeSpec::UnflattenImpl(const Span& leaves) const { - absl::InlinedVector agenda; + auto agenda = absl::InlinedVector{}; auto it = leaves.begin(); ssize_t leaf_count = 0; for (const Node& node : m_traversal) { @@ -34,7 +34,7 @@ py::object PyTreeSpec::UnflattenImpl(const Span& leaves) const { switch (node.kind) { case PyTreeKind::None: case PyTreeKind::Leaf: { - if (node.kind == PyTreeKind::Leaf || m_none_is_leaf) [[likely]] { // NOLINT + if (node.kind == PyTreeKind::Leaf || m_none_is_leaf) [[likely]] { if (it == leaves.end()) [[unlikely]] { throw std::invalid_argument( absl::StrFormat("Too few leaves for PyTreeSpec; expected %ld, got %ld.", From 96315894e3ec52a7433d28047dc793b7b3dd903f Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Tue, 24 Jan 2023 11:47:39 +0800 Subject: [PATCH 2/2] style(clang-tidy): sort checks --- .clang-tidy | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 6649f7ca..ee21712b 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -10,16 +10,16 @@ cppcoreguidelines-*, -cppcoreguidelines-avoid-non-const-global-variables, -cppcoreguidelines-macro-usage, -cppcoreguidelines-pro-type-reinterpret-cast, -hicpp-exception-baseclass, hicpp-avoid-goto, +hicpp-exception-baseclass, +misc-*, +-misc-const-correctness, modernize-*, -modernize-use-trailing-return-type, performance-*, readability-*, --readability-identifier-length, -readability-convert-member-functions-to-static, -misc-*, --misc-const-correctness, +-readability-identifier-length, ' HeaderFilterRegex: '^include/.*$' WarningsAsErrors: '*'