Skip to content
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

Properly constrain the basic_json conversion operator #2825

Merged
merged 1 commit into from
Jul 22, 2021
Merged

Properly constrain the basic_json conversion operator #2825

merged 1 commit into from
Jul 22, 2021

Conversation

ldionne
Copy link
Contributor

@ldionne ldionne commented Jun 17, 2021

Fixes #2491

This was reported as a libc++ bug in http://llvm.org/PR48507, but it's a bug in this library, not in libc++. See Richard Smith's explanation in http://llvm.org/PR48507.

@ldionne ldionne requested a review from nlohmann as a code owner June 17, 2021 17:32
@ldionne
Copy link
Contributor Author

ldionne commented Jun 17, 2021

I'm not sure what standards you support. If some of the type traits (e.g. std::conjunction) are not available on all the compilers you support, it's easy to fix that by implementing our own. Just let me know.

I'm now going to close http://llvm.org/PR48507 because it's not a libc++ bug.

Copy link

@alexezeder alexezeder left a comment

Choose a reason for hiding this comment

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

Just two comments to compare with my PR. The appearance of a new PR made me update mine. 😉

Comment on lines +42 to +43
template<template<class...> class Op, class... Args>
struct is_detected_lazy : is_detected<Op, Args...> { };

Choose a reason for hiding this comment

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

IMHO, That's done way better than in my PR. I like the idea of making the number of additional instantiations as small as possible, and this allows to eliminate unnecessary instantiations. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just two comments to compare with my PR. The appearance of a new PR made me update mine. 😉

Sorry, I didn't realize there was already a PR for this.

Comment on lines 3345 to 3373
std::conjunction<
std::negation<std::is_pointer<ValueType>>,
std::negation<std::is_same<ValueType, detail::json_ref<basic_json>>>,
std::negation<std::is_same<ValueType, typename string_t::value_type>>,
std::negation<detail::is_basic_json<ValueType>>,
std::negation<std::is_same<ValueType, std::initializer_list<typename string_t::value_type>>>,

#if defined(JSON_HAS_CPP_17) && (defined(__GNUC__) || (defined(_MSC_VER) && _MSC_VER >= 1910 && _MSC_VER <= 1914))
&& !std::is_same<ValueType, typename std::string_view>::value
std::negation<std::is_same<ValueType, std::string_view>>,
#endif
&& detail::is_detected<detail::get_template_function, const basic_json_t&, ValueType>::value
, int >::type = 0 >
detail::is_detected_lazy<detail::get_template_function, const basic_json_t&, ValueType>
>::value, int >::type = 0 >

Choose a reason for hiding this comment

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

IMHO, there is no much sense to use templated/typed/parametrized logic operations for all operands. For the same reason as above. Of course, it can backfire in the future, but it wouldn't hurt too much since these operands can be replaced pretty quickly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could technically do something like:

std::conjunction<
    std::bool_constant<
        !std::is_pointer<ValueType>::value &&
        !std::is_same<ValueType, detail::json_ref<basic_json>>::value &&
        !std::is_same<ValueType, typename string_t::value_type>::value &&
        !detail::is_basic_json<ValueType>::value &&
        !std::is_same<ValueType, std::initializer_list<typename string_t::value_type>>::value &&
        // ...
    >,
    detail::is_detected_lazy<detail::get_template_function, const basic_json_t&, ValueType>
>

But I don't think it provides a big benefit, and it's less consistent.

Choose a reason for hiding this comment

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

Yeah, actually, it's

json/include/nlohmann/json.hpp

Lines 3345 to 3357 in ae9cfb1

detail::conjunction <
std::integral_constant < bool,
!std::is_pointer<ValueType>::value&&
!std::is_same<ValueType, detail::json_ref<basic_json>>::value&&
!std::is_same<ValueType, typename string_t::value_type>::value&&
!detail::is_basic_json<ValueType>::value
&& !std::is_same<ValueType, std::initializer_list<typename string_t::value_type>>::value
#if defined(JSON_HAS_CPP_17) && (defined(__GNUC__) || (defined(_MSC_VER) && _MSC_VER >= 1910 && _MSC_VER <= 1914))
&& !std::is_same<ValueType, typename std::string_view>::value
#endif
>,
detail::is_detected<detail::get_template_function, const basic_json_t&, ValueType >>::value
, int >::type = 0 >

You are right about consistency for sure. Does it give a big benefit - I don't know, I'm just hoping that it helps. But looking more at this library, probably a few more instantiations wouldn't be noticeable.

@coveralls
Copy link

coveralls commented Jul 10, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling b0e5965 on ldionne:ldionne-lazy into 03270ef on nlohmann:develop.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Can you please update this branch to the latest develop branch?

test/src/unit-bug2491.cpp Outdated Show resolved Hide resolved
include/nlohmann/detail/meta/type_traits.hpp Outdated Show resolved Hide resolved
@ldionne
Copy link
Contributor Author

ldionne commented Jul 19, 2021

Rebased onto develop and renamed test as requested.

test/src/unit-regression3.cpp Outdated Show resolved Hide resolved
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

The test for the amalgamation (https://github.com/nlohmann/json/runs/3104916288?check_suite_focus=true) failed. Can you please run make amalgamate - this should re-indent and regenerate the single header.

@ldionne
Copy link
Contributor Author

ldionne commented Jul 19, 2021

The test for the amalgamation (https://github.com/nlohmann/json/runs/3104916288?check_suite_focus=true) failed. Can you please run make amalgamate - this should re-indent and regenerate the single header.

It looks like there's nothing to be done on my system for amalgamate:

% make amalgamate
make: Nothing to be done for `amalgamate'.

@nlohmann
Copy link
Owner

It looks like there's nothing to be done on my system for amalgamate:

% make amalgamate
make: Nothing to be done for `amalgamate'.

That is strange. Can you try to delete file single_include/nlohmann/json.hpp and re-run make amalgamate?

@ldionne
Copy link
Contributor Author

ldionne commented Jul 19, 2021

It looks like there's nothing to be done on my system for amalgamate:

% make amalgamate
make: Nothing to be done for `amalgamate'.

That is strange. Can you try to delete file single_include/nlohmann/json.hpp and re-run make amalgamate?

Now that seems to make a difference, thanks.

@nlohmann
Copy link
Owner

Thank you for your patience!

@nlohmann
Copy link
Owner

The remaining issue in the CI has been fixed in the develop branch, so technically this PR is ready to be merged.

As I am no expert in template magic, I do no really understand the fix. @gregmarr @alexezeder can either of you have a quick look whether the changes are OK to you?

@nlohmann nlohmann added state: please discuss please discuss the issue or vote for your favorite option review needed It would be great if someone could review the proposed changes. and removed state: please discuss please discuss the issue or vote for your favorite option labels Jul 22, 2021
@alexezeder
Copy link

@alexezeder can either of you have a quick look whether the changes are OK to you?

I cannot call myself an expert in template magic either, but since I understand what causes the errors here - the changes are OK to me. I've already reviewed this PR, and I agree with the @ldionne that it's not worth changing logic operations for all operands for consistency. Also, as @ldionne mentioned, it doesn't provide a big benefit.

@nlohmann nlohmann self-assigned this Jul 22, 2021
@nlohmann nlohmann added release item: 🐛 bug fix and removed review needed It would be great if someone could review the proposed changes. labels Jul 22, 2021
@nlohmann nlohmann added this to the Release 3.9.2 milestone Jul 22, 2021
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann merged commit a563338 into nlohmann:develop Jul 22, 2021
@nlohmann
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compilation error when using NLOHMANN_JSON_SERIALIZE_ENUM ordered_json on libc++
5 participants