feat(buffer_object): Add resize method to BufferIO#57
Conversation
Summary of ChangesHello @Leahlijuan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request introduces a resize method to the BufferIO class and the underlying C++ BufferObject. The implementation is generally sound, correctly managing memory-mapped regions and metadata. I have identified a few areas for improvement: specifically, validating the new size upfront in BufferIO.resize to ensure atomicity and adhering to the repository's testing style guide by using structured comments (Given/When/Then) in the new test cases.
tests/checkpoint_object_manager/buffer_object/buffer_helper_test.cpp
Outdated
Show resolved
Hide resolved
src/ml_flashpoint/checkpoint_object_manager/buffer_object/buffer_helper.cpp
Outdated
Show resolved
Hide resolved
src/ml_flashpoint/checkpoint_object_manager/buffer_object/buffer_helper.cpp
Outdated
Show resolved
Hide resolved
| out_data_ptr = ptr; | ||
| out_data_size = new_size; | ||
|
|
||
| LOG(INFO) << "Successfully resized mmap to " << new_size; |
There was a problem hiding this comment.
there is no LOG(DEBUG) for this library
There was a problem hiding this comment.
ah true, can we use VLOG(DEBUG_LEVEL) then, that's the recommendation. can set constexpr DEBUG_LEVEL = 4 .
There was a problem hiding this comment.
but for other methods, we use LOG(INFO) for similar logs, maybe we can keep it for now and modify the log in the future for all of them
src/ml_flashpoint/checkpoint_object_manager/buffer_object/buffer_helper.h
Outdated
Show resolved
Hide resolved
src/ml_flashpoint/checkpoint_object_manager/buffer_object/buffer_helper.cpp
Show resolved
Hide resolved
tests/checkpoint_object_manager/buffer_object/buffer_helper_test.cpp
Outdated
Show resolved
Hide resolved
b460288 to
9879a7e
Compare
| out_data_ptr = ptr; | ||
| out_data_size = new_size; | ||
|
|
||
| LOG(INFO) << "Successfully resized mmap to " << new_size; |
There was a problem hiding this comment.
ah true, can we use VLOG(DEBUG_LEVEL) then, that's the recommendation. can set constexpr DEBUG_LEVEL = 4 .
tests/checkpoint_object_manager/buffer_object/buffer_helper_test.cpp
Outdated
Show resolved
Hide resolved
tests/checkpoint_object_manager/buffer_object/buffer_helper_test.cpp
Outdated
Show resolved
Hide resolved
Python Code Coverage Summary
Minimum allowed line rate is |
C++ Code Coverage Summary
Minimum allowed line rate is |
Fixes #40