From 429dbb9d86503489624081c197ffd8b43743fa7e Mon Sep 17 00:00:00 2001 From: LinZhihao-723 Date: Sat, 23 Nov 2024 19:51:27 -0500 Subject: [PATCH 1/6] Fixing... --- src/clp_ffi_py/Py_utils.cpp | 42 ++++++++++++++++++++----------------- src/clp_ffi_py/Py_utils.hpp | 2 +- src/clp_ffi_py/Python.hpp | 1 + 3 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/clp_ffi_py/Py_utils.cpp b/src/clp_ffi_py/Py_utils.cpp index 266e0753..a6edfaa6 100644 --- a/src/clp_ffi_py/Py_utils.cpp +++ b/src/clp_ffi_py/Py_utils.cpp @@ -2,24 +2,28 @@ #include "Py_utils.hpp" +#include #include +#include + #include #include namespace clp_ffi_py { namespace { constexpr char const* const cPyFuncNameGetFormattedTimestamp{"get_formatted_timestamp"}; -PyObjectStaticPtr Py_func_get_formatted_timestamp{nullptr}; - constexpr char const* const cPyFuncNameGetTimezoneFromTimezoneId{"get_timezone_from_timezone_id"}; -PyObjectStaticPtr Py_func_get_timezone_from_timezone_id{nullptr}; - constexpr std::string_view cPyFuncNameSerializeDictToMsgpack{"serialize_dict_to_msgpack"}; -PyObjectStaticPtr Py_func_serialize_dict_to_msgpack{nullptr}; - constexpr std::string_view cPyFuncNameParseJsonStr{"parse_json_str"}; -PyObjectStaticPtr Py_func_parse_json_str{nullptr}; + +// NOLINTBEGIN(cppcoreguidelines-avoid-non-const-global-variables) +PyObjectStaticPtr py_func_get_formatted_timestamp{nullptr}; +PyObjectStaticPtr py_func_get_timezone_from_timezone_id{nullptr}; +PyObjectStaticPtr py_func_serialize_dict_to_msgpack{nullptr}; +PyObjectStaticPtr py_func_parse_json_str{nullptr}; + +// NOLINTEND(cppcoreguidelines-avoid-non-const-global-variables) /** * Wrapper of PyObject_CallObject. @@ -39,29 +43,29 @@ auto py_utils_init() -> bool { return false; } - Py_func_get_timezone_from_timezone_id.reset( + py_func_get_timezone_from_timezone_id.reset( PyObject_GetAttrString(py_utils, cPyFuncNameGetTimezoneFromTimezoneId) ); - if (nullptr == Py_func_get_timezone_from_timezone_id.get()) { + if (nullptr == py_func_get_timezone_from_timezone_id.get()) { return false; } - Py_func_get_formatted_timestamp.reset( + py_func_get_formatted_timestamp.reset( PyObject_GetAttrString(py_utils, cPyFuncNameGetFormattedTimestamp) ); - if (nullptr == Py_func_get_formatted_timestamp.get()) { + if (nullptr == py_func_get_formatted_timestamp.get()) { return false; } - Py_func_serialize_dict_to_msgpack.reset( + py_func_serialize_dict_to_msgpack.reset( PyObject_GetAttrString(py_utils, cPyFuncNameSerializeDictToMsgpack.data()) ); - if (nullptr == Py_func_serialize_dict_to_msgpack.get()) { + if (nullptr == py_func_serialize_dict_to_msgpack.get()) { return false; } - Py_func_parse_json_str.reset(PyObject_GetAttrString(py_utils, cPyFuncNameParseJsonStr.data())); - if (nullptr == Py_func_parse_json_str.get()) { + py_func_parse_json_str.reset(PyObject_GetAttrString(py_utils, cPyFuncNameParseJsonStr.data())); + if (nullptr == py_func_parse_json_str.get()) { return false; } @@ -75,7 +79,7 @@ auto py_utils_get_formatted_timestamp(clp::ir::epoch_time_ms_t timestamp, PyObje if (nullptr == func_args) { return nullptr; } - return py_utils_function_call_wrapper(Py_func_get_formatted_timestamp.get(), func_args); + return py_utils_function_call_wrapper(py_func_get_formatted_timestamp.get(), func_args); } auto py_utils_get_timezone_from_timezone_id(std::string const& timezone_id) -> PyObject* { @@ -84,7 +88,7 @@ auto py_utils_get_timezone_from_timezone_id(std::string const& timezone_id) -> P if (nullptr == func_args) { return nullptr; } - return py_utils_function_call_wrapper(Py_func_get_timezone_from_timezone_id.get(), func_args); + return py_utils_function_call_wrapper(py_func_get_timezone_from_timezone_id.get(), func_args); } auto py_utils_serialize_dict_to_msgpack(PyDictObject* py_dict) -> PyBytesObject* { @@ -95,7 +99,7 @@ auto py_utils_serialize_dict_to_msgpack(PyDictObject* py_dict) -> PyBytesObject* if (nullptr == func_args) { return nullptr; } - auto* result{py_utils_function_call_wrapper(Py_func_serialize_dict_to_msgpack.get(), func_args) + auto* result{py_utils_function_call_wrapper(py_func_serialize_dict_to_msgpack.get(), func_args) }; if (nullptr == result) { return nullptr; @@ -117,6 +121,6 @@ auto py_utils_parse_json_str(std::string_view json_str) -> PyObject* { if (nullptr == func_args) { return nullptr; } - return py_utils_function_call_wrapper(Py_func_parse_json_str.get(), func_args); + return py_utils_function_call_wrapper(py_func_parse_json_str.get(), func_args); } } // namespace clp_ffi_py diff --git a/src/clp_ffi_py/Py_utils.hpp b/src/clp_ffi_py/Py_utils.hpp index bd687661..4a3f1586 100644 --- a/src/clp_ffi_py/Py_utils.hpp +++ b/src/clp_ffi_py/Py_utils.hpp @@ -6,7 +6,7 @@ #include #include -#include +#include namespace clp_ffi_py { /** diff --git a/src/clp_ffi_py/Python.hpp b/src/clp_ffi_py/Python.hpp index 8d6451bd..83603264 100644 --- a/src/clp_ffi_py/Python.hpp +++ b/src/clp_ffi_py/Python.hpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include From 1431d928a70a3f0a89bd055fe36a4bc7fee083d3 Mon Sep 17 00:00:00 2001 From: LinZhihao-723 Date: Mon, 25 Nov 2024 23:34:37 -0500 Subject: [PATCH 2/6] Fix string view issue --- src/clp_ffi_py/Py_utils.cpp | 35 ++++++++++++++++++++++------------- src/clp_ffi_py/utils.hpp | 11 +++++++++++ 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/clp_ffi_py/Py_utils.cpp b/src/clp_ffi_py/Py_utils.cpp index a6edfaa6..a947763b 100644 --- a/src/clp_ffi_py/Py_utils.cpp +++ b/src/clp_ffi_py/Py_utils.cpp @@ -9,11 +9,12 @@ #include #include +#include namespace clp_ffi_py { namespace { -constexpr char const* const cPyFuncNameGetFormattedTimestamp{"get_formatted_timestamp"}; -constexpr char const* const cPyFuncNameGetTimezoneFromTimezoneId{"get_timezone_from_timezone_id"}; +constexpr std::string_view cPyFuncNameGetFormattedTimestamp{"get_formatted_timestamp"}; +constexpr std::string_view cPyFuncNameGetTimezoneFromTimezoneId{"get_timezone_from_timezone_id"}; constexpr std::string_view cPyFuncNameSerializeDictToMsgpack{"serialize_dict_to_msgpack"}; constexpr std::string_view cPyFuncNameParseJsonStr{"parse_json_str"}; @@ -43,28 +44,34 @@ auto py_utils_init() -> bool { return false; } - py_func_get_timezone_from_timezone_id.reset( - PyObject_GetAttrString(py_utils, cPyFuncNameGetTimezoneFromTimezoneId) - ); + py_func_get_timezone_from_timezone_id.reset(PyObject_GetAttrString( + py_utils, + get_c_str_from_constexpr_string_view(cPyFuncNameGetTimezoneFromTimezoneId) + )); if (nullptr == py_func_get_timezone_from_timezone_id.get()) { return false; } - py_func_get_formatted_timestamp.reset( - PyObject_GetAttrString(py_utils, cPyFuncNameGetFormattedTimestamp) - ); + py_func_get_formatted_timestamp.reset(PyObject_GetAttrString( + py_utils, + get_c_str_from_constexpr_string_view(cPyFuncNameGetFormattedTimestamp) + )); if (nullptr == py_func_get_formatted_timestamp.get()) { return false; } - py_func_serialize_dict_to_msgpack.reset( - PyObject_GetAttrString(py_utils, cPyFuncNameSerializeDictToMsgpack.data()) - ); + py_func_serialize_dict_to_msgpack.reset(PyObject_GetAttrString( + py_utils, + get_c_str_from_constexpr_string_view(cPyFuncNameSerializeDictToMsgpack) + )); if (nullptr == py_func_serialize_dict_to_msgpack.get()) { return false; } - py_func_parse_json_str.reset(PyObject_GetAttrString(py_utils, cPyFuncNameParseJsonStr.data())); + py_func_parse_json_str.reset(PyObject_GetAttrString( + py_utils, + get_c_str_from_constexpr_string_view(cPyFuncNameParseJsonStr) + )); if (nullptr == py_func_parse_json_str.get()) { return false; } @@ -116,7 +123,9 @@ auto py_utils_serialize_dict_to_msgpack(PyDictObject* py_dict) -> PyBytesObject* } auto py_utils_parse_json_str(std::string_view json_str) -> PyObject* { - PyObjectPtr const func_args_ptr{Py_BuildValue("(s)", json_str.data())}; + PyObjectPtr const func_args_ptr{ + Py_BuildValue("(s#)", json_str.data(), static_cast(json_str.size())) + }; auto* func_args{func_args_ptr.get()}; if (nullptr == func_args) { return nullptr; diff --git a/src/clp_ffi_py/utils.hpp b/src/clp_ffi_py/utils.hpp index 3554034a..3c92f554 100644 --- a/src/clp_ffi_py/utils.hpp +++ b/src/clp_ffi_py/utils.hpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -85,6 +86,16 @@ auto handle_traceable_exception(clp::TraceableException& exception) noexcept -> template [[maybe_unused]] constexpr bool cAlwaysFalse{false}; +/** + * A consteval to get the C-string from a constexpr string view. + * @param sv + * @return The underlying C-string of the given constexpr string view. + */ +[[nodiscard]] consteval auto get_c_str_from_constexpr_string_view(std::string_view const& sv +) -> char const* { + return sv.data(); +} + template auto parse_py_int(PyObject* py_int, int_type& val) -> bool { if (false == static_cast(PyLong_Check(py_int))) { From 89f8ea014e8e903f0fa5a9f7f5031835dc9b7a91 Mon Sep 17 00:00:00 2001 From: LinZhihao-723 Date: Tue, 26 Nov 2024 12:57:32 -0500 Subject: [PATCH 3/6] Add project wise clang-tidy config --- src/clp_ffi_py/.clang-tidy | 6 ++++++ src/clp_ffi_py/Py_utils.cpp | 32 ++++++++++++++++---------------- 2 files changed, 22 insertions(+), 16 deletions(-) create mode 100644 src/clp_ffi_py/.clang-tidy diff --git a/src/clp_ffi_py/.clang-tidy b/src/clp_ffi_py/.clang-tidy new file mode 100644 index 00000000..947aee37 --- /dev/null +++ b/src/clp_ffi_py/.clang-tidy @@ -0,0 +1,6 @@ +InheritParentConfig: true + +CheckOptions: + # Variable naming rules + # NOTE: we set this to allow PyObject global variables to start with `Py_` + readability-identifier-naming.GlobalVariableIgnoredRegexp: "Py_.*" diff --git a/src/clp_ffi_py/Py_utils.cpp b/src/clp_ffi_py/Py_utils.cpp index a947763b..9e13e36b 100644 --- a/src/clp_ffi_py/Py_utils.cpp +++ b/src/clp_ffi_py/Py_utils.cpp @@ -19,10 +19,10 @@ constexpr std::string_view cPyFuncNameSerializeDictToMsgpack{"serialize_dict_to_ constexpr std::string_view cPyFuncNameParseJsonStr{"parse_json_str"}; // NOLINTBEGIN(cppcoreguidelines-avoid-non-const-global-variables) -PyObjectStaticPtr py_func_get_formatted_timestamp{nullptr}; -PyObjectStaticPtr py_func_get_timezone_from_timezone_id{nullptr}; -PyObjectStaticPtr py_func_serialize_dict_to_msgpack{nullptr}; -PyObjectStaticPtr py_func_parse_json_str{nullptr}; +PyObjectStaticPtr Py_func_get_formatted_timestamp{nullptr}; +PyObjectStaticPtr Py_func_get_timezone_from_timezone_id{nullptr}; +PyObjectStaticPtr Py_func_serialize_dict_to_msgpack{nullptr}; +PyObjectStaticPtr Py_func_parse_json_str{nullptr}; // NOLINTEND(cppcoreguidelines-avoid-non-const-global-variables) @@ -44,35 +44,35 @@ auto py_utils_init() -> bool { return false; } - py_func_get_timezone_from_timezone_id.reset(PyObject_GetAttrString( + Py_func_get_timezone_from_timezone_id.reset(PyObject_GetAttrString( py_utils, get_c_str_from_constexpr_string_view(cPyFuncNameGetTimezoneFromTimezoneId) )); - if (nullptr == py_func_get_timezone_from_timezone_id.get()) { + if (nullptr == Py_func_get_timezone_from_timezone_id.get()) { return false; } - py_func_get_formatted_timestamp.reset(PyObject_GetAttrString( + Py_func_get_formatted_timestamp.reset(PyObject_GetAttrString( py_utils, get_c_str_from_constexpr_string_view(cPyFuncNameGetFormattedTimestamp) )); - if (nullptr == py_func_get_formatted_timestamp.get()) { + if (nullptr == Py_func_get_formatted_timestamp.get()) { return false; } - py_func_serialize_dict_to_msgpack.reset(PyObject_GetAttrString( + Py_func_serialize_dict_to_msgpack.reset(PyObject_GetAttrString( py_utils, get_c_str_from_constexpr_string_view(cPyFuncNameSerializeDictToMsgpack) )); - if (nullptr == py_func_serialize_dict_to_msgpack.get()) { + if (nullptr == Py_func_serialize_dict_to_msgpack.get()) { return false; } - py_func_parse_json_str.reset(PyObject_GetAttrString( + Py_func_parse_json_str.reset(PyObject_GetAttrString( py_utils, get_c_str_from_constexpr_string_view(cPyFuncNameParseJsonStr) )); - if (nullptr == py_func_parse_json_str.get()) { + if (nullptr == Py_func_parse_json_str.get()) { return false; } @@ -86,7 +86,7 @@ auto py_utils_get_formatted_timestamp(clp::ir::epoch_time_ms_t timestamp, PyObje if (nullptr == func_args) { return nullptr; } - return py_utils_function_call_wrapper(py_func_get_formatted_timestamp.get(), func_args); + return py_utils_function_call_wrapper(Py_func_get_formatted_timestamp.get(), func_args); } auto py_utils_get_timezone_from_timezone_id(std::string const& timezone_id) -> PyObject* { @@ -95,7 +95,7 @@ auto py_utils_get_timezone_from_timezone_id(std::string const& timezone_id) -> P if (nullptr == func_args) { return nullptr; } - return py_utils_function_call_wrapper(py_func_get_timezone_from_timezone_id.get(), func_args); + return py_utils_function_call_wrapper(Py_func_get_timezone_from_timezone_id.get(), func_args); } auto py_utils_serialize_dict_to_msgpack(PyDictObject* py_dict) -> PyBytesObject* { @@ -106,7 +106,7 @@ auto py_utils_serialize_dict_to_msgpack(PyDictObject* py_dict) -> PyBytesObject* if (nullptr == func_args) { return nullptr; } - auto* result{py_utils_function_call_wrapper(py_func_serialize_dict_to_msgpack.get(), func_args) + auto* result{py_utils_function_call_wrapper(Py_func_serialize_dict_to_msgpack.get(), func_args) }; if (nullptr == result) { return nullptr; @@ -130,6 +130,6 @@ auto py_utils_parse_json_str(std::string_view json_str) -> PyObject* { if (nullptr == func_args) { return nullptr; } - return py_utils_function_call_wrapper(py_func_parse_json_str.get(), func_args); + return py_utils_function_call_wrapper(Py_func_parse_json_str.get(), func_args); } } // namespace clp_ffi_py From 0a89250b43d3d1144b6fc85b80af1d190f509bfa Mon Sep 17 00:00:00 2001 From: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com> Date: Tue, 26 Nov 2024 12:58:03 -0500 Subject: [PATCH 4/6] Update src/clp_ffi_py/utils.hpp Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com> --- src/clp_ffi_py/utils.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/clp_ffi_py/utils.hpp b/src/clp_ffi_py/utils.hpp index 3c92f554..ad6ae2ba 100644 --- a/src/clp_ffi_py/utils.hpp +++ b/src/clp_ffi_py/utils.hpp @@ -87,7 +87,6 @@ template [[maybe_unused]] constexpr bool cAlwaysFalse{false}; /** - * A consteval to get the C-string from a constexpr string view. * @param sv * @return The underlying C-string of the given constexpr string view. */ From d595a37e3a38de106941614fe892999795af3f09 Mon Sep 17 00:00:00 2001 From: LinZhihao-723 Date: Tue, 26 Nov 2024 13:28:12 -0500 Subject: [PATCH 5/6] Fix lint task --- lint-tasks.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lint-tasks.yml b/lint-tasks.yml index 6c004666..de4038a8 100644 --- a/lint-tasks.yml +++ b/lint-tasks.yml @@ -103,7 +103,9 @@ tasks: cmds: - task: ":utils:clang-tidy" vars: - FLAGS: "--config-file={{.ROOT_DIR}}/.clang-tidy -p {{.CLP_FFI_PY_COMPILE_COMMANDS_DB}}" + FLAGS: > + --config-file={{.CLP_FFI_PY_CPP_SRC_DIR}}/.clang-tidy + -p {{.CLP_FFI_PY_COMPILE_COMMANDS_DB}} SRC_PATHS: ref: ".G_CPP_LINT_DIRS" VENV_DIR: "{{.G_LINT_VENV_DIR}}" From 06a50d36726c7de614c2f0e5c7bab6ccaf60cf0b Mon Sep 17 00:00:00 2001 From: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com> Date: Tue, 26 Nov 2024 15:11:38 -0500 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com> --- lint-tasks.yml | 6 +++--- src/clp_ffi_py/.clang-tidy | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lint-tasks.yml b/lint-tasks.yml index de4038a8..362bee30 100644 --- a/lint-tasks.yml +++ b/lint-tasks.yml @@ -103,9 +103,9 @@ tasks: cmds: - task: ":utils:clang-tidy" vars: - FLAGS: > - --config-file={{.CLP_FFI_PY_CPP_SRC_DIR}}/.clang-tidy - -p {{.CLP_FFI_PY_COMPILE_COMMANDS_DB}} + FLAGS: >- + --config-file "{{.CLP_FFI_PY_CPP_SRC_DIR}}/.clang-tidy" + -p "{{.CLP_FFI_PY_COMPILE_COMMANDS_DB}}" SRC_PATHS: ref: ".G_CPP_LINT_DIRS" VENV_DIR: "{{.G_LINT_VENV_DIR}}" diff --git a/src/clp_ffi_py/.clang-tidy b/src/clp_ffi_py/.clang-tidy index 947aee37..66d28440 100644 --- a/src/clp_ffi_py/.clang-tidy +++ b/src/clp_ffi_py/.clang-tidy @@ -2,5 +2,5 @@ InheritParentConfig: true CheckOptions: # Variable naming rules - # NOTE: we set this to allow PyObject global variables to start with `Py_` - readability-identifier-naming.GlobalVariableIgnoredRegexp: "Py_.*" + # Allow PyObject global variables to start with `Py_` + readability-identifier-naming.GlobalVariableIgnoredRegexp: "Py(_[a-z0-9]+)+"