Suppress Clang-Tidy warning

[nlohmann]

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

[coveralls]

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

[github-actions[bot]]

🔴 Amalgamation check failed! 🔴

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

[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).