Refactor serialization implementation with new C++23 features #651
Conversation
tigercosmos
force-pushed
the
refactor_json
branch
from
9c5cf83 to
86625d0
Compare
| template <typename T> | ||
| concept JsonSerializableConcept = std::is_base_of_v<SerializableItem, T>; | ||
|
|
||
| template <typename T> | ||
| concept JsonStringConcept = std::is_convertible_v<T, std::string>; | ||
|
|
||
| template <typename T> | ||
| concept JsonBooleanConcept = std::is_same_v<T, bool>; | ||
|
|
||
| template <typename T> | ||
| concept JsonIntegralConcept = std::is_integral_v<T> && !JsonBooleanConcept<T>; | ||
|
|
||
| template <typename T> | ||
| concept JsonVectorConcept = is_specialization_of_v<std::vector, T>; | ||
|
|
||
| template <typename T> | ||
| concept JsonMapConcept = is_specialization_of_v<std::unordered_map, T> && std::is_same_v<typename T::key_type, std::string>; |
There was a problem hiding this comment.
Named concepts are clear for understanding.
| else if constexpr (JsonStringConcept<T>) | ||
| { | ||
| return "\"" + escape_string(value) + "\""; | ||
| return std::format("\"{}\"", escape_string(value)); |
There was a problem hiding this comment.
Using std::format is more clear.
| oss << separator << to_json_string(item); /* recursive here */ | ||
| return to_json_string(item); /* recursive here */ | ||
| }; | ||
| auto json_items = value | std::views::transform(transform_function); |
There was a problem hiding this comment.
Fancy way to collect all JSON items.
There was a problem hiding this comment.
It reads clearer to throw the lambda into the function argument without a name:
auto json_items = value | std::views::transform([](const auto & item){ return to_json_string(item); });But I prefer the old looping style since it is way easier to insert break point. Your choice, but I bet someone (or you) will change it back to loop in some day.
| value = true; | ||
| } | ||
| } | ||
| else if constexpr (std::is_same_v<T, std::string>) |
There was a problem hiding this comment.
Removed duplicated case. Didn't catch this error before.
| auto & obj = std::get<detail::JsonMap>(node->value); | ||
| for (const auto & [key, jsonNode] : obj) | ||
| const auto & obj = std::get<JsonMap>(node->value); | ||
| for (const auto & [key, json_node] : obj) |
There was a problem hiding this comment.
Followed coding style.
| } | ||
| auto & str = std::get<std::string>(node->value); | ||
| value = str.substr(1, str.size() - 2); /* Remove quotes */ | ||
| value = std::get<std::string>(node->value) != "false"; |
There was a problem hiding this comment.
AI is smart here.
| keys.push_back(kv.first); | ||
| } | ||
| std::sort(keys.begin(), keys.end()); // TODO: the sorting may not be necessary. This is more for the testing purpose. | ||
| auto keys = value | std::views::keys | std::ranges::to<std::vector>(); |
There was a problem hiding this comment.
Fancy way to collect keys.
|
@yungyuc Please help take a look. Thanks! |
|
It seems that GCC 13.3 doesn't fully support C++23. Wait #652. |
I moved it back to draft. Make it ready for review when it is ready for review. |
tigercosmos
force-pushed
the
refactor_json
branch
from
86625d0 to
d5d8ba8
Compare
|
@yungyuc ready for review. |
yungyuc
left a comment
yungyuc
left a comment
There was a problem hiding this comment.
Concept is not a good idea in this case.
- Revert to the original type trait because the concept style wipes out the exact type names and makes code less readable.
| std::string JsonHelper::to_json_string(const T & value) | ||
| { | ||
| if constexpr (std::is_base_of_v<SerializableItem, T>) | ||
| if constexpr (JsonSerializableConcept<T>) |
There was a problem hiding this comment.
The original type trait is more readable:
std::is_base_of_v<SerializableItem, T>
std::is_convertible_v<T, std::string>
...The concept style wipes out the exact type names. It's bad. Revert to the original type trait.
| oss << separator << to_json_string(item); /* recursive here */ | ||
| return to_json_string(item); /* recursive here */ | ||
| }; | ||
| auto json_items = value | std::views::transform(transform_function); |
There was a problem hiding this comment.
It reads clearer to throw the lambda into the function argument without a name:
auto json_items = value | std::views::transform([](const auto & item){ return to_json_string(item); });But I prefer the old looping style since it is way easier to insert break point. Your choice, but I bet someone (or you) will change it back to loop in some day.
2 participants
Refactor serialization implementation with new C++23 features.
Using Copilot + Claude 4.5 as the first shot, and manually fixed all details.
Blocked by #652