-
Notifications
You must be signed in to change notification settings - Fork 13
refactor(master): Modernize master-metalogger comm #659
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
da49791 to
c76da6d
Compare
48ebf31 to
c821dc8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modernizes the matomlserv.cc file by refactoring it from C-style code to modern C++. The changes replace manual memory management with RAII patterns, use STL containers instead of raw pointers and manual linked lists, and adopt modern C++ conventions throughout.
Key changes include:
- Replaced manual memory management and linked lists with
std::deque,std::queue, andstd::vector - Introduced
InputPacketandOutputPacketwrapper classes for safer packet handling - Converted C-style strings to
std::stringand enums toenum class - Modernized logging from
safs_pretty_syslogtosafs::log_*functions - Replaced macros with
constexprconstants
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is an excellent step towards modernizing the master-metalogger communication service. The transition from C-style manual memory management to C++ containers like std::deque, std::vector, and std::string greatly improves code readability, maintainability, and safety by leveraging RAII. The use of enum class and constexpr is also a welcome improvement.
While the refactoring is very well done, I've identified a critical issue where a cleanup function is not called, leading to resource leaks and potential dangling pointers. I've also found a few minor issues related to incorrect logging format strings and a degradation in error reporting. My detailed comments are below.
c821dc8 to
2b1b5a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is looking great. Please see my comments.
2b1b5a8 to
2a50605
Compare
Previous version of the master to metalogger service communication was using an old C-styled code, which made this implementation hard to maintain and understand. Also, previous manual memory assigment and free was performed manually, which is an error-prone approach. This commit modernizes the master to metaloggers service by moving to a C++-style based code. Signed-off-by: Rolando Sánchez Ramos <[email protected]>
2a50605 to
1d87fec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Great job.
Previous version of the master to metalogger service communication was using an old C-styled code, which made this implementation hard to maintain and understand. Also, previous manual memory assigment and free was performed manually, which is an error-prone approach. This commit modernizes the master to metaloggers service by moving to a C++-style based code.