refactor: Fix clang-tidy warnings for the rest of files directly under src/clp_ffi_py; Use std::string_view for static error message strings.
#102
Conversation
WalkthroughThe pull request introduces various changes across multiple files, primarily enhancing error handling, updating type definitions, and refining linting tasks. Key modifications include the addition of source files to the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
lint-tasks.yml(1 hunks)src/clp_ffi_py/PyObjectCast.hpp(3 hunks)src/clp_ffi_py/error_messages.hpp(1 hunks)src/clp_ffi_py/ir/native/PyDeserializerBuffer.cpp(1 hunks)src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp(1 hunks)src/clp_ffi_py/ir/native/PyLogEvent.cpp(7 hunks)src/clp_ffi_py/ir/native/PyMetadata.cpp(3 hunks)src/clp_ffi_py/ir/native/PyQuery.cpp(8 hunks)src/clp_ffi_py/ir/native/PySerializer.cpp(2 hunks)src/clp_ffi_py/ir/native/deserialization_methods.cpp(2 hunks)src/clp_ffi_py/utils.hpp(2 hunks)
🧰 Additional context used
📓 Learnings (3)
src/clp_ffi_py/ir/native/deserialization_methods.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#94
File: src/clp_ffi_py/ir/native/PyDeserializer.cpp:218-242
Timestamp: 2024-11-23T05:55:57.106Z
Learning: In `src/clp_ffi_py/ir/native/PyDeserializer.cpp`, the `deserialize_log_event` method in the `PyDeserializer` class ensures proper loop termination and does not risk infinite loops, even when handling incomplete or corrupted streams.
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp (3)
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#81
File: src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.hpp:0-0
Timestamp: 2024-11-14T06:30:18.577Z
Learning: In `PyKeyValuePairLogEvent`, we assume invalid objects will never be created, so null pointer checks in methods like `get_kv_pair_log_event()` are unnecessary, assuming CPython system runs correctly.
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#90
File: src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp:653-675
Timestamp: 2024-11-16T03:42:40.523Z
Learning: In `src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp`, within the function `PyKeyValuePairLogEvent::to_dict`, only `clp::TraceableException` needs to be caught, as no other `std::exception`s can be thrown.
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#81
File: src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.hpp:42-44
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In the C++ codebase, for consistency with other classes, methods like `get_kv_pair_log_event()` in `src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.hpp` should return pointers instead of references.
src/clp_ffi_py/ir/native/PySerializer.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#83
File: src/clp_ffi_py/ir/native/PySerializer.hpp:73-76
Timestamp: 2024-11-19T01:13:58.505Z
Learning: In the `PySerializer` class in `src/clp_ffi_py/ir/native/PySerializer.hpp`, the `close_serializer()` method never fails, so additional error handling in the `clean()` method is unnecessary.
🔇 Additional comments (21)
src/clp_ffi_py/error_messages.hpp (2)
4-4: LGTM! Excellent transition to std::string_view
The transition from const char* const to std::string_view for error messages is a good modernization that provides:
- Zero-cost string operations
- Compile-time string handling
- Better type safety
Also applies to: 7-15
13-15: Good catch on fixing the typo in the constant name
Fixed typo: cTimezoneObjectNotInitialzed → cTimezoneObjectNotInitialized
src/clp_ffi_py/utils.hpp (1)
134-136: LGTM! Efficient implementation of string_view conversion
The consteval implementation ensures compile-time evaluation and zero runtime overhead.
lint-tasks.yml (1)
112-119: LGTM! The added source files align with the PR objectives.
The addition of these source files to the cpp-static-check task's SRC_PATHS ensures they will be checked by clang-tidy, which is consistent with the PR's goal of fixing clang-tidy warnings.
Let's verify these files exist:
✅ Verification successful
Let me verify the remaining files from the list that weren't checked in the previous script.
All source files are present and correctly located
All the specified source files have been verified to exist in the src/clp_ffi_py directory:
- api_decoration.hpp
- error_messages.hpp
- ExceptionFFI.hpp
- Py_utils.cpp
- Py_utils.hpp
- PyExceptionContext.hpp
- PyObjectCast.hpp
- PyObjectUtils.hpp
Note: While there is an additional error_messages.hpp in src/clp_ffi_py/ir/native/, this doesn't affect the validity of the files being added to the cpp-static-check task.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the newly added source files
# Test: Check if all the files exist. Expect: All files should be found.
for file in "api_decoration.hpp" "error_messages.hpp" "ExceptionFFI.hpp" "PyExceptionContext.hpp" "PyObjectCast.hpp" "PyObjectUtils.hpp"; do
fd --type f "^${file}$" src/clp_ffi_py
done
Length of output: 1391
Script:
#!/bin/bash
# Check for Py_utils.cpp and Py_utils.hpp
fd --type f "^Py_utils\.(cpp|hpp)$" src/clp_ffi_py
Length of output: 108
src/clp_ffi_py/ir/native/PyMetadata.cpp (1)
142-145: LGTM! Consistent improvement in error message handling.
The changes properly transition error message handling to use get_c_str_from_constexpr_string_view, which aligns with the PR's objective of using std::string_view for static error messages. This approach is more efficient and type-safe compared to raw string literals.
Also applies to: 233-236, 252-255
src/clp_ffi_py/ir/native/deserialization_methods.cpp (1)
187-187: LGTM! Consistent improvement in type error handling.
The changes properly transition type error messages to use get_c_str_from_constexpr_string_view, maintaining consistency with the PR's objective of using std::string_view for static error messages. The existing error handling logic remains robust, as confirmed by previous learnings about proper loop termination in related code.
Also applies to: 332-332
src/clp_ffi_py/ir/native/PyDeserializerBuffer.cpp (1)
388-391: LGTM! Consistent error message handling.
The change properly utilizes get_c_str_from_constexpr_string_view for error message handling, aligning with the PR's objective of using std::string_view for static strings.
src/clp_ffi_py/ir/native/PyLogEvent.cpp (4)
67-70: LGTM! Proper error message handling in type check.
The change correctly uses get_c_str_from_constexpr_string_view for the type error message.
181-184: LGTM! Consistent error handling in setstate method.
The changes properly update all error messages in the setstate method to use get_c_str_from_constexpr_string_view, maintaining consistency across the codebase.
Also applies to: 190-194, 206-206, 218-222, 232-236
322-322: LGTM! Simplified error message handling.
The change consolidates the error message handling into a single line while maintaining the same functionality.
499-502: LGTM! Proper error handling in init method.
The change correctly uses get_c_str_from_constexpr_string_view for the out-of-memory error message.
src/clp_ffi_py/ir/native/PySerializer.cpp (2)
8-8: LGTM! Added necessary header.
The addition of the <new> header is appropriate for dynamic memory allocation operations in the code.
407-410: LGTM! Consistent error message handling.
The change properly uses get_c_str_from_constexpr_string_view for the out-of-memory error message, maintaining consistency with other files.
src/clp_ffi_py/ir/native/PyQuery.cpp (7)
38-41: LGTM: Consistent error handling approach
The change properly utilizes get_c_str_from_constexpr_string_view for error message handling, aligning with the codebase-wide standardization.
50-53: LGTM: Consistent error handling
The error handling follows the same pattern, maintaining consistency throughout the function.
284-287: LGTM: Proper error handling for invalid state
The error handling correctly uses the standardized approach for reporting invalid state dictionary errors.
295-297: LGTM: Proper key error handling
The error handling for missing dictionary keys follows the standardized approach.
314-317: LGTM: Consistent key error handling
The error handling maintains consistency with other key error checks in the function.
331-335: LGTM: Proper wildcard queries error handling
The error handling for the wildcard_queries key follows the established pattern.
636-642: LGTM: Essential null check after allocation
The immediate null check after allocation is a crucial safety measure, properly using the standardized error handling approach.
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp (1)
647-650: LGTM: Proper memory allocation error handling
The error handling for memory allocation failure follows the standardized approach using get_c_str_from_constexpr_string_view.
Description
This is one of the PR series trying to implement #96
This PR fixes all clang-tidy warnings for the rest of the files directly under
src/clp_ffi_py(not the inner directories).Also, it refactors
error_messages.hppto usestd::string_viewto replace the use ofchar const*for constexpr static strings. This change propagates to the files that use these error messages, to update them to use string_view instead.Validation performed
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores