From 8e3de0a3e71f35cbe0c1ad24c15308127cb9cd21 Mon Sep 17 00:00:00 2001 From: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com> Date: Mon, 6 Jan 2025 13:53:43 -0500 Subject: [PATCH] refactor: Fix clang-tidy warnings in `PyKeyValuePairLogEvent`. (#108) Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com> --- lint-tasks.yml | 5 ++ src/clp_ffi_py/.clang-tidy | 4 +- .../ir/native/PyKeyValuePairLogEvent.cpp | 50 ++++++++++++------- src/clp_ffi_py/utils.cpp | 4 ++ src/clp_ffi_py/utils.hpp | 7 +++ src/wrapped_facade_headers/msgpack.hpp | 3 ++ 6 files changed, 54 insertions(+), 19 deletions(-) diff --git a/lint-tasks.yml b/lint-tasks.yml index 9d7635b..f9f6e83 100644 --- a/lint-tasks.yml +++ b/lint-tasks.yml @@ -102,8 +102,11 @@ tasks: cmds: - task: ":utils:clang-tidy" vars: + # NOTE: clang-tidy raises a warning in msgpack's source code, unrelated to this project, + # so we suppress the warning using a line filter until clang-tidy or msgpack is updated. FLAGS: >- --config-file "{{.CLP_FFI_PY_CPP_SRC_DIR}}/.clang-tidy" + --line-filter="[{\"name\":\"cpp11_zone.hpp\",\"lines\":[[197,197]]}]" -p "{{.CLP_FFI_PY_COMPILE_COMMANDS_DB}}" SRC_PATHS: # TODO: Before all clang-tidy violations are resolved, we should only run clang-tidy on @@ -112,6 +115,8 @@ tasks: - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/api_decoration.hpp" - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/error_messages.hpp" - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/ExceptionFFI.hpp" + - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/ir/native/PyKeyValuePairLogEvent.cpp" + - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/ir/native/PyKeyValuePairLogEvent.hpp" - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/modules" - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/Py_utils.cpp" - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/Py_utils.hpp" diff --git a/src/clp_ffi_py/.clang-tidy b/src/clp_ffi_py/.clang-tidy index b492a3b..dcf2db0 100644 --- a/src/clp_ffi_py/.clang-tidy +++ b/src/clp_ffi_py/.clang-tidy @@ -7,4 +7,6 @@ Checks: CheckOptions: # Variable naming rules # Allow PyObject global variables to start with `Py_` - readability-identifier-naming.GlobalVariableIgnoredRegexp: "Py(_[a-z0-9]+)+" + readability-identifier-naming.GlobalVariableIgnoredRegexp: "Py([a-zA-Z0-9]*)(_[a-z0-9]+)+" + readability-identifier-naming.FunctionIgnoredRegexp: "Py([a-zA-z0-9]*)(_[a-z0-9]+)+" + readability-identifier-naming.GlobalFunctionIgnoredRegexp: "Py([a-zA-z0-9]*)(_[a-z0-9]+)+" diff --git a/src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp b/src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp index c57b79d..2190be9 100644 --- a/src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp +++ b/src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include @@ -173,10 +174,16 @@ class PyDictSerializationIterator { ); return false; } + PyObjectPtr const py_key{ + construct_py_str_from_string_view(m_schema_tree_node->get_key_name()) + }; + if (nullptr == py_key) { + return false; + } return 0 - == PyDict_SetItemString( + == PyDict_SetItem( py_reinterpret_cast(m_parent_py_dict), - m_schema_tree_node->get_key_name().data(), + py_key.get(), py_reinterpret_cast(m_py_dict.get()) ); } @@ -263,7 +270,7 @@ CLP_FFI_PY_METHOD auto PyKeyValuePairLogEvent_to_dict(PyKeyValuePairLogEvent* se */ CLP_FFI_PY_METHOD auto PyKeyValuePairLogEvent_dealloc(PyKeyValuePairLogEvent* self) -> void; -// NOLINTNEXTLINE(cppcoreguidelines-avoid-c-arrays) +// NOLINTNEXTLINE(*-avoid-c-arrays, cppcoreguidelines-avoid-non-const-global-variables) PyMethodDef PyKeyValuePairLogEvent_method_table[]{ {"to_dict", py_c_function_cast(PyKeyValuePairLogEvent_to_dict), @@ -273,7 +280,8 @@ PyMethodDef PyKeyValuePairLogEvent_method_table[]{ {nullptr} }; -// NOLINTBEGIN(cppcoreguidelines-avoid-c-arrays, cppcoreguidelines-pro-type-*-cast) +// NOLINTBEGIN(*-avoid-c-arrays, cppcoreguidelines-pro-type-*-cast) +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) PyType_Slot PyKeyValuePairLogEvent_slots[]{ {Py_tp_alloc, reinterpret_cast(PyType_GenericAlloc)}, {Py_tp_dealloc, reinterpret_cast(PyKeyValuePairLogEvent_dealloc)}, @@ -283,11 +291,12 @@ PyType_Slot PyKeyValuePairLogEvent_slots[]{ {Py_tp_doc, const_cast(static_cast(cPyKeyValuePairLogEventDoc))}, {0, nullptr} }; -// NOLINTEND(cppcoreguidelines-avoid-c-arrays, cppcoreguidelines-pro-type-*-cast) +// NOLINTEND(*-avoid-c-arrays, cppcoreguidelines-pro-type-*-cast) /** * `PyKeyValuePairLogEvent`'s Python type specifications. */ +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) PyType_Spec PyKeyValuePairLogEvent_type_spec{ "clp_ffi_py.ir.native.KeyValuePairLogEvent", sizeof(PyKeyValuePairLogEvent), @@ -426,15 +435,19 @@ auto convert_py_dict_to_key_value_pair_log_event(PyDictObject* py_dict) if (serializer_result.has_error()) { PyErr_Format( PyExc_RuntimeError, - cSerializerCreateErrorFormatStr.data(), + get_c_str_from_constexpr_string_view(cSerializerCreateErrorFormatStr), serializer_result.error().message().c_str() ); return std::nullopt; } auto& serializer{serializer_result.value()}; + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-union-access) if (false == serializer.serialize_msgpack_map(msgpack_obj.via.map)) { - PyErr_SetString(PyExc_RuntimeError, cSerializerSerializeMsgpackMapError.data()); + PyErr_SetString( + PyExc_RuntimeError, + get_c_str_from_constexpr_string_view(cSerializerSerializeMsgpackMapError) + ); return std::nullopt; } @@ -450,7 +463,7 @@ auto convert_py_dict_to_key_value_pair_log_event(PyDictObject* py_dict) if (deserializer_result.has_error()) { PyErr_Format( PyExc_RuntimeError, - cDeserializerCreateErrorFormatStr.data(), + get_c_str_from_constexpr_string_view(cDeserializerCreateErrorFormatStr), deserializer_result.error().message().c_str() ); return std::nullopt; @@ -463,7 +476,9 @@ auto convert_py_dict_to_key_value_pair_log_event(PyDictObject* py_dict) if (result.has_error()) { PyErr_Format( PyExc_RuntimeError, - cDeserializerDeserializeNextIrUnitErrorFormatStr.data(), + get_c_str_from_constexpr_string_view( + cDeserializerDeserializeNextIrUnitErrorFormatStr + ), result.error().message().c_str() ); return std::nullopt; @@ -548,13 +563,17 @@ auto serialize_node_id_value_pair_to_py_dict( std::optional const& optional_val, PyDictObject* dict ) -> bool { - auto const key_name{node.get_key_name()}; + PyObjectPtr const py_key{construct_py_str_from_string_view(node.get_key_name())}; + if (nullptr == py_key) { + return false; + } + if (false == optional_val.has_value()) { PyObjectPtr const empty_dict{PyDict_New()}; return 0 - == PyDict_SetItemString( + == PyDict_SetItem( py_reinterpret_cast(dict), - key_name.data(), + py_key.get(), empty_dict.get() ); } @@ -617,12 +636,7 @@ auto serialize_node_id_value_pair_to_py_dict( return false; } - return 0 - == PyDict_SetItemString( - py_reinterpret_cast(dict), - key_name.data(), - py_value.get() - ); + return 0 == PyDict_SetItem(py_reinterpret_cast(dict), py_key.get(), py_value.get()); } auto decode_as_encoded_text_ast(Value const& val) -> std::optional { diff --git a/src/clp_ffi_py/utils.cpp b/src/clp_ffi_py/utils.cpp index c57d72b..49ebc4b 100644 --- a/src/clp_ffi_py/utils.cpp +++ b/src/clp_ffi_py/utils.cpp @@ -95,4 +95,8 @@ auto handle_traceable_exception(clp::TraceableException& exception) noexcept -> exception.what() ); } + +auto construct_py_str_from_string_view(std::string_view sv) -> PyObject* { + return PyUnicode_FromStringAndSize(sv.data(), static_cast(sv.size())); +} } // namespace clp_ffi_py diff --git a/src/clp_ffi_py/utils.hpp b/src/clp_ffi_py/utils.hpp index 816a13f..92d1311 100644 --- a/src/clp_ffi_py/utils.hpp +++ b/src/clp_ffi_py/utils.hpp @@ -84,6 +84,13 @@ template */ auto handle_traceable_exception(clp::TraceableException& exception) noexcept -> void; +/** + * @param sv + * @return A new reference to the constructed Python string object from the given string view `sv`. + * @Return nullptr on failure with the relevant Python exception and error set. + */ +[[nodiscard]] auto construct_py_str_from_string_view(std::string_view sv) -> PyObject*; + /** * A template that always evaluates as false. */ diff --git a/src/wrapped_facade_headers/msgpack.hpp b/src/wrapped_facade_headers/msgpack.hpp index 248d024..1c603c7 100644 --- a/src/wrapped_facade_headers/msgpack.hpp +++ b/src/wrapped_facade_headers/msgpack.hpp @@ -7,10 +7,13 @@ // Inform IWYU of the headers that we use that are exported by msgpack.hpp // IWYU pragma: begin_exports #include +#include #include "msgpack/v1/unpack_decl.hpp" #include +#include #include "msgpack/v2/unpack_decl.hpp" #include +#include #include "msgpack/v3/unpack_decl.hpp" // IWYU pragma: end_exports #endif