Conversation
|
@pgrAm Could you review/verify? Thanks for the report. |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #2549 where custom type serialization using C++20 tag_invoke was broken in the JSON builder. The fix removes the erroneous custom_deserializable constraint (which is for deserialization, not serialization) and properly implements forwarding references to support both lvalue and rvalue custom types.
Key changes:
- Fixed
serialize_tag::operator()to use forwarding references and remove incorrect constraint - Updated
string_builder::append()to properly forward arguments totag_invoke - Added conditional compilation support for C++26
#embedfeature to prevent compilation errors when the feature is unavailable
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
include/simdjson/generic/ondemand/json_string_builder.h |
Removed incorrect custom_deserializable constraint and changed serialize_tag::operator() and append() to use forwarding references |
include/simdjson/generic/ondemand/json_string_builder-inl.h |
Updated append() implementation to correctly call tag_invoke with forwarding |
tests/builder/builder_string_builder_tests.cpp |
Added Car2549 struct and issue2549() test to verify custom serialization via tag_invoke works correctly |
tests/compile_time/compile_time_json_tests.cpp |
Wrapped C++26 #embed feature usage in conditional compilation to prevent errors on compilers without support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Changes look about right, I'll give it a try |
|
Can confirm, @lemire your branch appears to be working for me |
|
@pgrAm Much appreciated. |
Short title (summary):
Fixing issue #2549
Description
The JSON builder should have allowed custom types using C++20 tag_invoke, but the feature was broken. Sadly.
Type of change