Suppress Clang-Tidy warning

[nlohmann]

modernize-use-designated-initializers does not work with C++11

[coveralls]

coverage: 100.0%. remained the same
when pulling 4b7721c on clang-tidy
into 0457de2 on develop.

[github-actions]

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @nlohmann
Please read and follow the Contribution Guidelines.

[gregmarr]

Can we just do assert_invariant(false); in the body (assert it in this after moving instead of asserting in other) to avoid this mess?

[gregmarr]

Another possibility for avoiding all these issues is to do std::swap() on m_data and other.m_data in the body.

[nlohmann]

I am not at all happy with this. I also thought about swapping...

[fsandhei]

Sorry for asking a question for something that is probably obvious, but which constructor of json_base_class_t is this call valid for? I suppose it is one of the basic_json constructors being used here?

FWIW, it should be completely fine to check the invariants after the initializer list has been evaluated. Is there a possibility to consider rewriting the data move constructor so these assignments don't need to be done at this level?

[nlohmann]

I am currently blocked here:

template < typename BasicJsonType, typename T, std::size_t... Idx >
std::array<T, sizeof...(Idx)> from_json_inplace_array_impl(BasicJsonType&& j,
        identity_tag<std::array<T, sizeof...(Idx)>> /*unused*/, index_sequence<Idx...> /*unused*/)
{
    return { { std::forward<BasicJsonType>(j).at(Idx).template get<T>()... } };
}

gives

/__w/json/json/include/nlohmann/detail/conversions/from_json.hpp:280:44: error: 'j' used after it was forwarded [bugprone-use-after-move,hicpp-invalid-access-moved,-warnings-as-errors]
  280 |     return { { std::forward<BasicJsonType>(j).at(Idx).template get<T>()... } };
      |                                            ^
/__w/json/json/include/nlohmann/detail/conversions/from_json.hpp:280:47: note: forward occurred here
  280 |     return { { std::forward<BasicJsonType>(j).at(Idx).template get<T>()... } };
      |                                               ^
/__w/json/json/include/nlohmann/detail/conversions/from_json.hpp:280:44: note: the use happens in a later loop iteration than the forward
  280 |     return { { std::forward<BasicJsonType>(j).at(Idx).template get<T>()... } };
      |                                            ^

Any ideas?

[gregmarr]

Every other from_json function except for auto from_json(BasicJsonType&& j, TupleRelated&& t) takes const BasicJsonType &. If this was the same, that eliminates the need for the std::forward.

Alternatively, if there is a defined need for moving from the json object into an std::array, then have separate const BasicJsonType & and BasicJsonType && functions and put the && on the array element type: template get<T&&>() in the latter.

[nlohmann]

I made some progress and now have an interesting issue:

template<class KeyType, detail::enable_if_t<
             detail::is_usable_as_basic_json_key_type<basic_json_t, KeyType>::value, int> = 0>
reference at(KeyType && key)
{
    auto it = m_data.m_value.object->find(std::forward<KeyType>(key));
    if (it == m_data.m_value.object->end())
    {
        JSON_THROW(out_of_range::create(403, detail::concat("key '", string_t(std::forward<KeyType>(key)), "' not found"), this)); // <-- warning here: 'key' used after it was forwarded
    }
    return set_parent(it->second);
}

To fix this, I could either make a copy of key which defeats the purpose of taking KeyType && in the first place - or to dumb down the error message and not mention key. What do you think?

[fsandhei]

I made some progress and now have an interesting issue:

template<class KeyType, detail::enable_if_t<
             detail::is_usable_as_basic_json_key_type<basic_json_t, KeyType>::value, int> = 0>
reference at(KeyType && key)
{
    auto it = m_data.m_value.object->find(std::forward<KeyType>(key));
    if (it == m_data.m_value.object->end())
    {
        JSON_THROW(out_of_range::create(403, detail::concat("key '", string_t(std::forward<KeyType>(key)), "' not found"), this)); // <-- warning here: 'key' used after it was forwarded
    }
    return set_parent(it->second);
}

To fix this, I could either make a copy of key which defeats the purpose of taking KeyType && in the first place - or to dumb down the error message and not mention key. What do you think?

If key is an rvalue this would indeed be a double move so that could be unfortunate behavior. key is already "stolen" in the call above.

If key is an lvalue this is completely fine. Are there any clear downsides of constraining this to be just for lvalues?

I suppose that if you'd like to keep this to be a universal reference the exception message should perhaps not contain key so you don't try to move an already-moved object.

[gregmarr]

I would say don't mention key in the error message. About the only thing I can think you might be able to do here would be to split const KeyType & vs KeyType && and only put key in the message in the lvalue case.

[nlohmann]

We already have two versions for const references and rvalues. Unfortunately, it seems to depend on the compiler and C++ standard which version is used, so I am currently struggling to adjust the test cases (with and without key in the exception).