feat(cpp): add map type support#1590
Conversation
…ix rustfmt wit::string lacked comparison operators, causing compilation failures when used as a std::map key. Also fixes rustfmt formatting issues.
std::map keys are const, but the ABI lowering needs to call leak() on string keys to transfer ownership to the flat buffer. Use const_cast since the map is consumed during the lowering operation.
|
cc @cpetig and @TartanLlama |
const_cast fails when the map key type differs between contexts (e.g. std::string_view in imports vs wit::string in exports). Copying by value works universally and is safe since the map is consumed.
wasm-component-ld bundled with wasi-sdk 30 doesn't support the map type encoding (0x63) in the component model binary format. The C++ map codegen is still validated by the codegen test. The runtime test can be re-added when wasi-sdk ships a compatible wasm-component-ld.
| let size = entry.size.format(POINTER_SIZE_EXPRESSION); | ||
| let align = entry.align.format(POINTER_SIZE_EXPRESSION); | ||
| self.push_str(&format!("auto&& {val} = {};\n", operands[0])); | ||
| self.push_str(&format!("auto {len} = (size_t)({val}.size());\n")); |
There was a problem hiding this comment.
Can we avoid the C-style cast here?
| self.push_str(&format!("auto {len} = (size_t)({val}.size());\n")); | ||
| uwriteln!( | ||
| self.src, | ||
| "auto {ptr} = ({ptr_type})({len} > 0 ? cabi_realloc(nullptr, 0, {align}, {len} * {size}) : nullptr);", |
There was a problem hiding this comment.
Can we avoid the C-style cast here?
| let body_value = &body.1[1]; | ||
| uwriteln!( | ||
| self.src, | ||
| "{result}.insert(std::make_pair({}, {}));", |
There was a problem hiding this comment.
I think .emplace would be better here
| self.include("<bit>"); | ||
| } | ||
| if self.dependencies.needs_map { | ||
| self.include("<map>"); |
There was a problem hiding this comment.
I wonder if it would be better to have some configurability here? As a user, I'd often want to avoid std::map for performance reasons, and if I'm really perf-focused, I'd also avoid std::unordered_map and use some custom cache-friendly hash map type. Do you think we can enable this use-case without mandating an additional copy of the data?
There was a problem hiding this comment.
It seems that all the allowed key types from the spec would be hashable, so at least it may be better to default to std::unordered_map
| ); | ||
| uwriteln!(self.src, "size_t {idx} = 0;"); | ||
| uwriteln!(self.src, "for (auto& {entry_name} : {val}) {{"); | ||
| uwriteln!(self.src, "auto iter_map_key = {entry_name}.first;"); |
There was a problem hiding this comment.
Is there a reason we're copying the key and taking a reference to the value? Can we at least move the key?
| uwriteln!(self.src, "++{idx};"); | ||
| uwriteln!(self.src, "}}"); | ||
| if realloc.is_some() { | ||
| // ownership transfers |
There was a problem hiding this comment.
I'm not sure what this comment means or what this block is for
| let entry = self.r#gen.sizes.record([*key, *value]); | ||
| let size = entry.size.format(POINTER_SIZE_EXPRESSION); | ||
| uwriteln!(self.src, "uint8_t* base = {ptr} + {i} * {size};"); | ||
| uwriteln!(self.src, "(void) base;"); |
There was a problem hiding this comment.
Why do we need this line? In case the body doesn't use base?
|
@TartanLlama addressing your comments, the "why" behind things, I am effectively Junior in C++, please be patience with me here 🙏🏻 I am try to understand the intent and I will give it a second path |
|
If I were to implement this feature I would introduce a wit::map type which has the same layout as the lowered data. This way the data doesn't need to be copied unless the user code requests it (making map a lower cost abstraction). On the other hand I just ran into an adjacent problem with list lowering (and avoiding re-allocation) in #1592 , so avoiding a copy in case of complex types might still be impossible - due to C++ type layout restrictions. A |
std::map requires per-entry rb-tree allocation during lift; wit::map mirrors the canonical ABI layout (flat pair array) so lift is a single allocation, matching the existing wit::vector / wit::string pattern. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
wit::map::size() and std::span::size() already return size_t, so the C-style cast was a no-op. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Avoids an unused `base` local and the `(void) base;` suppression when the element has no nested heap-owned fields (e.g. list<u32>, map<char, u32>). Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The cast converts void* (from cabi_realloc) to uint8_t*, which is a well-defined static_cast and doesn't need a C-style cast. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
|
@cpetig @TartanLlama can you give it a pass again? 🙏🏻 Ideally, please share the intent of what you would like to see, I can figure out eventually the implementation but I am not qualify to the best practices or insights of C++. I can figure that out for sure, it would help to declare the intent so I can go off, study, and figure out what you want 😄 |
4 participants
This PR adds map type support to the C++ backend, following the same pattern established in #1562 (core), #1583 (Go), and #1584 (MoonBit).