Fix capnproto add_value_to_map overwriting all entries at index 0#634
Fix capnproto add_value_to_map overwriting all entries at index 0#634lixin-wei wants to merge 1 commit intogetml:mainfrom
Conversation
add_value_to_map constructed a fresh OutputArrayType each call without using _parent->ix_++, so ix_ always defaulted to 0 and every map entry overwrote slot 0 in the Cap'n Proto entries list. Only the last entry survived serialization; on read-back the remaining default entries (with empty-string keys) triggered stoull errors. Fix by using designated initializers with .ix_ = _parent->ix_++, matching all other add_*_to_map methods in Writer.cpp. Also wrap the key _name in std::string() so add_value_to_object deduces T as std::string rather than std::string_view, which has no handling branch. Add test_map_value_types to exercise maps with primitive value types (std::map<std::string, std::string>) through add_value_to_map. Made-with: Cursor
Summary of ChangesHello, 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 addresses a critical bug in the Cap'n Proto Highlights
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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the add_value_to_map function in Writer.hpp to correctly initialize OutputArrayType with an index (.ix_ = _parent->ix_++) and explicitly convert std::string_view keys to std::string for proper Cap'n Proto serialization. A new test file, test_map_value_types.cpp, was added to validate the serialization and deserialization of maps containing string values, confirming the intended functionality of the rfl::capnproto library.
|
The failed checks seems not related to my change. It's a network issue. |
1 participant
Summary
Fix a bug in capnproto's add_value_to_map, It doesn't use parent->ix++ to track the entry index. Instead, it creates a new OutputArrayType each time with ix_ defaulting to 0, so every entry overwrites index 0 in the capnproto list.
Test plan
capnproto.test_map_with_string_valuesfails before the fix (only last entry survives round-trip)