build: Add headers specification in CMakeLists.txt and remove spdlog in public headers - #46
Conversation
…and remove spdlog dependency in header
WalkthroughThis pull request introduces changes to the build system configuration and source code in the Spider project. The modifications primarily focus on updating the CMake configuration for header file management, adding a new function for processing message pack buffers, and standardizing header file include paths. The changes enhance the organization of header files, improve error handling in the new function, and streamline the way header files are referenced across multiple files in the project. Changes
Possibly Related PRs
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: 0
🧹 Nitpick comments (1)
src/spider/worker/FunctionManager.cpp (1)
64-100: Consider pre-allocating result_buffers vector.The implementation is well-structured with proper error handling and logging. However, we could optimize memory allocations by pre-allocating the result_buffers vector since we know its size from object.via.array.size.
- std::vector<msgpack::sbuffer> result_buffers; + std::vector<msgpack::sbuffer> result_buffers; + result_buffers.reserve(object.via.array.size - 1);🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/spider/CMakeLists.txt(2 hunks)src/spider/worker/FunctionManager.cpp(2 hunks)src/spider/worker/FunctionManager.hpp(1 hunks)tests/worker/worker-test.cpp(1 hunks)tests/worker/worker-test.hpp(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/worker/worker-test.hpp
- tests/worker/worker-test.cpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/spider/worker/FunctionManager.cpp
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: non-storage-unit-tests
- GitHub Check: lint
🔇 Additional comments (4)
src/spider/worker/FunctionManager.cpp (1)
8-8: LGTM! Appropriate placement of spdlog include.The addition of spdlog include in the source file aligns with the PR objective of removing spdlog from public headers while maintaining logging functionality.
src/spider/worker/FunctionManager.hpp (1)
164-165: LGTM! Appropriate separation of declaration and implementation.Moving the implementation to the source file while maintaining the declaration in the header is a good practice that aligns with the PR objective of cleaning up header dependencies.
src/spider/CMakeLists.txt (2)
35-42: LGTM! Modern CMake usage with FILE_SET.The change to use CMake's FILE_SET feature is a good modernization that improves header organization and installation.
160-167: LGTM! Consistent FILE_SET usage across targets.The parallel update to spider_client's header specification maintains consistency in the build system configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/spider/worker/FunctionManager.cpp (1)
67-103: Consider enhancing error messages with more contextThe error handling is thorough, but the error messages could be more descriptive by including the actual array size when reporting "Wrong type" error.
Apply this diff to improve the error message:
- spdlog::error("Cannot split result into buffers: Wrong type"); + spdlog::error("Cannot split result into buffers: Wrong type or size (expected >= 2, got {})", + object.via.array.size);🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/spider/worker/FunctionManager.cpp(2 hunks)src/spider/worker/FunctionManager.hpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/spider/worker/FunctionManager.cpp
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: non-storage-unit-tests
- GitHub Check: lint
🔇 Additional comments (4)
src/spider/worker/FunctionManager.cpp (2)
3-11: Well-organized include directives!The includes are properly organized and grouped by their categories (standard library, third-party, and project headers).
Line range hint
1-103: Implementation successfully moved from header to source fileThe changes effectively achieve the PR objective of moving the implementation to the source file while maintaining consistent coding patterns and error handling.
🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/spider/worker/FunctionManager.hpp (2)
163-164: Header changes align with PR objectivesSuccessfully removed the spdlog dependency from the public header while maintaining a clean interface.
Line range hint
1-164: Well-maintained header organizationThe header file maintains a clean public interface while properly organizing template implementations and declarations.
1 participant
Description
As title. Move more functions from
FunctionManager.hpptoFunctionManager.cppto remove dependencies onspdlog.Validation performed
Summary by CodeRabbit
Release Notes
Build System
FILE_SETfor managing header files inspider_coreandspider_clientlibraries.New Features
response_get_result_buffersfunction to process message pack buffers with error handling.Code Improvements
response_get_result_buffersfunction.These changes enhance the project's build process and code structure while maintaining existing functionality.