Skip to content

Allow using rfl::AddTagsToVariants with rfl::Generic and same-name-different-namespace structs #467

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jmcken8 wants to merge 3 commits into from
Closed

Allow using rfl::AddTagsToVariants with rfl::Generic and same-name-different-namespace structs #467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef5e781
Allow using `rfl::AddTagsToVariants` with rfl::Generic and same-name …
jmcken8 Jul 18, 2025
a7fd29e
Merge branch 'main' into f/bugfix_generic_and_variant_added_tags
liuzicheng1987 Sep 7, 2025
db148fb
Handle std::string edge case
liuzicheng1987 Sep 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions include/rfl/parsing/VariantAlternativeWrapper.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,12 @@ template <class T>
consteval auto make_tag() {
if constexpr (internal::has_tag_v<T>) {
return typename T::Tag();

} else if constexpr (std::is_same_v<std::remove_cvref_t<T>, std::string>) {
return Literal<"std::string">();

} else {
return Literal<
internal::remove_namespaces<internal::get_type_name<T>()>()>();
return Literal<internal::get_type_name<T>()>();
}
}

Expand Down
4 changes: 1 addition & 3 deletions tests/json/test_add_tag_to_rfl_variant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
#include <cassert>
#include <iostream>
#include <rfl.hpp>
#include <rfl/json.hpp>
#include <string>
#include <variant>
#include <vector>

#include "write_and_read.hpp"
Expand Down Expand Up @@ -34,6 +32,6 @@ TEST(json, test_add_tag_to_rfl_variant) {

write_and_read<rfl::AddTagsToVariants>(
vec,
R"([{"button_pressed_t":{}},{"button_released_t":{"button":4}},{"key_pressed":{"key":99}},{"int":3}])");
R"([{"test_add_tag_to_rfl_variant::button_pressed_t":{}},{"test_add_tag_to_rfl_variant::button_released_t":{"button":4}},{"key_pressed":{"key":99}},{"int":3}])");
}
Copy link
Copy Markdown
Contributor

@bryceschober bryceschober Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Add another test to explicitly demonstrate how to avoid any namespace decoration in the serialization.

Suggested change
}
// Alternatively, in order to avoid getting the namespace to every alternative in our auto-tagged
// variant, we must manually tag all variant alternatives to control all serialized names
struct button_pressed_tagged_t {
using Tag = rfl::Literal<"button_pressed">;
};
struct button_released_tagged_t {
using Tag = rfl::Literal<"button_released">;
rfl::Box<int> button;
};
using my_tagged_event_type_t =
rfl::Variant<button_pressed_tagged_t, button_released_tagged_t, key_pressed_t, int>;
TEST(json, test_add_manual_tags_to_rfl_variants) {
std::vector<my_tagged_event_type_t> vec;
vec.emplace_back(button_pressed_tagged_t{});
vec.emplace_back(button_released_tagged_t{rfl::make_box<int>(4)});
vec.emplace_back(key_pressed_t{'c'});
vec.emplace_back(3);
write_and_read<rfl::AddTagsToVariants>(
vec,
R"([{"button_pressed":{}},{"button_released":{"button":4}},{"key_pressed":{"key":99}},{"int":3}])");
}

} // namespace test_add_tag_to_rfl_variant
47 changes: 45 additions & 2 deletions tests/json/test_add_tag_to_variant.cpp
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@bryceschober bryceschober Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Overall, I think it would be helpful to maintain the test structure of test_add_tag_to_variant.cpp and test_add_tag_to_rfl_variant.cpp in parallel as much as possible, because otherwise they kinda imply that they have different functionality.

Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#include <cassert>
#include <iostream>
#include <rfl.hpp>
#include <rfl/json.hpp>
#include <string>
Expand All @@ -10,6 +9,8 @@

namespace test_add_tag_to_variant {

// test 1 -> normal behaviour

struct button_pressed_t {};

struct button_released_t {};
Expand All @@ -22,12 +23,54 @@ struct key_pressed_t {
using my_event_type_t =
std::variant<button_pressed_t, button_released_t, key_pressed_t, int>;

// test 2 -> 'Generic' within a struct like this cannot be read/written
// due to the underlying `std::variant` holding two fields with name 'Generic'
// once 'remove_namespaces' is applied to the type name extraction (which is removed)
// in the MR this test is added
Comment thread
jmcken8 marked this conversation as resolved.
struct APIResult {
rfl::Generic result;
};
struct APIError {
rfl::Generic error;
};

using APICallOutput = rfl::Variant<APIResult, APIError>;
Copy link
Copy Markdown
Contributor

@bryceschober bryceschober Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix: I think this was meant to be std::variant:

Suggested change
using APICallOutput = rfl::Variant<APIResult, APIError>;
using APICallOutput = std::variant<APIResult, APIError>;


// test 3 -> two structs with the same name in different namespaces should still
// be serializable

namespace Result {
struct Message {
std::string result;
Copy link
Copy Markdown
Contributor

@bryceschober bryceschober Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Make a note of the alternative approach using rfl::Tag:

Suggested change
std::string result;
// Alternatively, manually tag instead of getting the namespaced name:
// rfl::Tag<"result_message">
std::string result;

};
} // namespace Result

namespace Error {
struct Message {
std::string error;
Copy link
Copy Markdown
Contributor

@bryceschober bryceschober Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Make a note of the alternative approach using rfl::Tag:

Suggested change
std::string error;
// Alternatively, manually tag instead of getting the namespaced name:
// rfl::Tag<"error_message">
std::string error;

int error_id;
};
}; // namespace Error

using Messages = std::variant<Result::Message, Error::Message>;

TEST(json, test_add_tag_to_variant) {
const auto vec = std::vector<my_event_type_t>(
{button_pressed_t{}, button_released_t{}, key_pressed_t{'c'}, 3});

write_and_read<rfl::AddTagsToVariants>(
vec,
R"([{"button_pressed_t":{}},{"button_released_t":{}},{"key_pressed":{"key":99}},{"int":3}])");
R"([{"test_add_tag_to_variant::button_pressed_t":{}},{"test_add_tag_to_variant::button_released_t":{}},{"key_pressed":{"key":99}},{"int":3}])");
}

TEST(json, test_add_tag_to_variant_with_generic) {
APICallOutput output = APIResult{"200"};
write_and_read<rfl::AddTagsToVariants>(output,
R"({"test_add_tag_to_variant::APIResult":{"result":{"std::string":"200"}}})");
}
TEST(json, test_add_tag_to_variant_different_namespaces) {
Messages m = Error::Message{.error="an error", .error_id=2};
write_and_read<rfl::AddTagsToVariants>(m,
R"({"test_add_tag_to_variant::Error::Message":{"error":"an error","error_id":2}})");
Comment thread
jmcken8 marked this conversation as resolved.
}
} // namespace test_add_tag_to_variant
Loading