-
-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Fix NLOHMANN_DEFINE_TYPE_* macros for zero-member types #4994
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.
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
base: develop
Are you sure you want to change the base?
Conversation
7d0683d to
7a86cf1
Compare
| #define NLOHMANN_JSON_FROM(v1) nlohmann_json_j.at(#v1).get_to(nlohmann_json_t.v1); | ||
| #define NLOHMANN_JSON_FROM_WITH_DEFAULT(v1) nlohmann_json_t.v1 = !nlohmann_json_j.is_null() ? nlohmann_json_j.value(#v1, nlohmann_json_default_obj.v1) : nlohmann_json_default_obj.v1; | ||
|
|
||
| #define NLOHMANN_JSON_TO_DEFAULT nlohmann_json_j = nlohmann::json::parse("{}"); (void) nlohmann_json_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should use the namespace or use the parse function, but should just use the object() function using the known local object: nlohmann_json_t::object().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh right, since in this context BasicJsonType is the JSON type should i change it to this?
#define NLOHMANN_JSON_TO_DEFAULT nlohmann_json_j = BasicJsonType::object(); (void) nlohmann_json_t;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, if that's available, then yes, that would be better!
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated. @chelseadzdr |
c1d2aa4 to
556a8a6
Compare
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated. @chelseadzdr |
Signed-off-by: chelseadzdr <[email protected]>
Signed-off-by: chelseadzdr <[email protected]>
Signed-off-by: chelseadzdr <[email protected]>
45e580f to
8ebada5
Compare
|
|
||
| // Macros to simplify conversion from/to types | ||
|
|
||
| #define NLOHMANN_JSON_EXPAND_ARGS(...) __VA_ARGS__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not great to have all those #ifdef _MSC_VER variants just for this:
MSC:
friend void to_json(BasicJsonType& nlohmann_json_j, const Type& nlohmann_json_t) { NLOHMANN_JSON_EXPAND(NLOHMANN_JSON_PASTE(NLOHMANN_JSON_TO, NLOHMANN_JSON_EXPAND_ARGS(__VA_ARGS__))) }
The rest:
friend void to_json(BasicJsonType& nlohmann_json_j, const Type& nlohmann_json_t) { NLOHMANN_JSON_EXPAND(NLOHMANN_JSON_PASTE(NLOHMANN_JSON_TO, ##__VA_ARGS__)) } \
Is there no syntax that works for all compilers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, glad it just worked!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI is failing with NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT. I tried a more aggressive change to see if it would pass, but the same thing happened. It expands to NLOHMANN_JSON_PASTE2 when no arguments are provided.
I’m out of ideas on how to fix this.
Maybe having separate branches for some compilers could solve it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it needs to be a different macro for empty classes. Can it get away with just two, one for intrusive and one for non-intrusive?
#define NLOHMANN_DEFINE_EMPTY_TYPE_INTRUSIVE(Type) \
template<typename BasicJsonType, nlohmann::detail::enable_if_t<nlohmann::detail::is_basic_json<BasicJsonType>::value, int> = 0> \
friend void to_json(BasicJsonType& nlohmann_json_j, const Type&) {
nlohmann_json_j = BasicJsonType.object();\
} \
template<typename BasicJsonType, nlohmann::detail::enable_if_t<nlohmann::detail::is_basic_json<BasicJsonType>::value, int> = 0> \
friend void from_json(const BasicJsonType&, Type&) { }
#define NLOHMANN_DEFINE_EMPTY_TYPE_NON_INTRUSIVE(Type) \
template<typename BasicJsonType, nlohmann::detail::enable_if_t<nlohmann::detail::is_basic_json<BasicJsonType>::value, int> = 0> \
void to_json(BasicJsonType& nlohmann_json_j, const Type&) { nlohmann_json_j = BasicJsonType::object(); } \
template<typename BasicJsonType, nlohmann::detail::enable_if_t<nlohmann::detail::is_basic_json<BasicJsonType>::value, int> = 0> \
void from_json(const BasicJsonType&, Type&) { }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would simplify compatibility a lot. If that´s possible then it should be added to the Docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would require a new docs entry. @nlohmann what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no macro that fits with MSVC and all other compilers, then I guess a new one would be better than having none at all - at least less surprising than one that only works for some compilers. And I guess those few people that actually need a macro for this case will probably find it.
Signed-off-by: chelseadzdr <[email protected]>
Signed-off-by: chelseadzdr <[email protected]>
Signed-off-by: chelseadzdr <[email protected]>
This pull request solves the issue #4041 using the changes made in the closed PR #4323
make amalgamate.When any of the NLOHMANN_DEFINE_TYPE_* macros are used with no data members, the preprocessor currently expands VA_ARGS to an empty token. This leads to invalid code which breaks compilation for empty types.
This PR adjusts the macro_scope so that the zero-member case is handled explicitly. It finishes the work made in #4323 keeping the new changes like "Generate template functions with NLOHMANN_DEFINE_TYPE macros (#4597)" and to keep the documentations added.
Because develop has moved on since #4323, the changes were reapplied and adjusted manually rather than via a direct merge.
Tests has been included.