[REACTOR] Cleanup and improve map array containers#462
Conversation
Summary of ChangesHello @tqchen, 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 significantly refactors the internal implementation of array and map container classes. The primary goal is to improve modularity and maintainability by separating common map functionalities into a new base class, removing an underutilized array base class, and introducing new utility methods. These changes consolidate shared logic and simplify the container's internal structure without altering external API behavior. 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
|
|
To simplify the review, the PR is structured in two commits
|
There was a problem hiding this comment.
Code Review
This pull request provides a good cleanup and refactoring of the map and array containers. Removing InplaceArrayBase simplifies the array implementation. Extracting the map implementation into map_base.h with MapBaseObj is a great step towards better modularity and reusability, allowing different map specializations. The addition of the clear() method is also a useful enhancement. The code changes look solid overall. I've found a couple of minor points regarding confusing static_asserts in the new map_base.h that could be improved for better maintainability.
1c7f998 to
f8d669d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This PR is a nice refactoring of the map and array containers. Removing InplaceArrayBase and extracting the map implementation into a new map_base.h file are good steps towards better modularity and code clarity. The changes across the modified files are consistent with this goal.
However, I've identified a critical issue in the instantiation logic for DenseMapBaseObj within the new map_base.h file. This issue will likely cause a compilation failure due to an incorrect assumption about object sizes and the use of make_object on a non-registered type. Please see the detailed comment for more information.
f8d669d to
e09fa8e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a nice refactoring of the map and array containers. The removal of InplaceArrayBase simplifies the class hierarchy, and extracting the map implementation into map_base.h improves modularity. The changes are consistent and well-executed. I have one minor suggestion to restore a sanity check that was lost during the refactoring.
This PR runs the following refactor of the map and array containers.
We also reorganized SmallBaseMapObj after DenseMapObj as SmallBaseMapObj is a specialization, which can help possible future refactors to make SmallBaseMapObj be aware of DenseMapObj.
The split out of map_base.h is roughly manual copy and a few extra manual edits to reduce unexpected surprises.