Optimize copy / assignment and size of json iterator

[sjanel]

Hello,

I stambled accross this clang-tidy warning in my project and investigated further, it being a json::const_iterator:

the parameter 'it' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

It looks like json::iterator is actually non trivially copyable (and quite big), which is unexpected for an iterator and the reason why clang-tidy is complaining. In templated code it is common practice to pass iterators by value and not by reference. After investigation and reading of the code I realized that it was made like that at the beginning as a workaround for a MSVC bug (see https://github.com/nlohmann/json/issues/1608). It is sad to impact all other compilation modes and compilers just for a MSVC bug.

I redesigned internal_iterator_t with two distinct implementations, one optimized in the case primitive_iterator_t, object_t::iterator and array_t::iterator are all trivially copyable (standard case, mainly for non-MSVC compilers), one for the general case (mainly used by MSVC, and other compilers with exotic vector and/or map iterator implementations).

This is the fix that I propose, in the hope that since then, the MSVC issue is not a concern anymore. After this PR, all operations on iterators are expected to be faster and iterators can be passed by value without any performance impact like standard operators.

[nlohmann]

Thanks! It seems @falbrechtskirchinger is working in #3446 on similar issues - can you please check whether his adjustments go in the same direction?

[coveralls]

Coverage remained the same at 100.0% when pulling 49badfcb442103886925f1d109b1adf935cd7f9c on sjanel:feature/defaultcopyconstructibleiterator into b21c34517900c46a943c804d9af3a20cd0c77062 on nlohmann:develop.

[falbrechtskirchinger]

• Remove default value initialization, which does not seem to be used, such that union is valid
• Made iter_impl copy construction default, such that it is trivially copyable (did the same for copy assignment)

Some of this is probably redundant due to my pending PR. Will look at the code in a second. Edit: Actually I hardly touched iter_impl my fixes target proxy iteration.

This is the fix that I propose, in the hope that since then, the MSVC issue is not a concern anymore. After this PR, all operations on iterators are expected to be faster and iterators can be passed by value without any performance impact like standard operators.

MSVC still can't handle it: "error C2338: union cannot be safely used with non trivial types"

But I agree that pessimizing this for everyone else is bad practice.

PS: I don't know why, but formatter slightly changes output of the file here and there... is it coming from my clang-format version (I use v14) ? Please help me setting my environment with the expected formatter settings

This project uses Artistic Style to format code which is executed by make amalgamate or make pretty. You should disable clang-format when working on this code.

[sjanel]

Thanks! It seems @falbrechtskirchinger is working in #3446 on similar issues - can you please check whether his adjustments go in the same direction?

Thanks for your comment. I checked the PR and it's not exactly the same thing, it's more about preparing for C++20 support with ranges. This PR is mainly an optimization of the json::iterator type.

[sjanel]

• Remove default value initialization, which does not seem to be used, such that union is valid
• Made iter_impl copy construction default, such that it is trivially copyable (did the same for copy assignment)

Some of this is probably redundant due to my pending PR. Will look at the code in a second. Edit: Actually I hardly touched iter_impl my fixes target proxy iteration.

This is the fix that I propose, in the hope that since then, the MSVC issue is not a concern anymore. After this PR, all operations on iterators are expected to be faster and iterators can be passed by value without any performance impact like standard operators.

MSVC still can't handle it: "error C2338: union cannot be safely used with non trivial types"

But I agree that pessimizing this for everyone else is bad practice.

PS: I don't know why, but formatter slightly changes output of the file here and there... is it coming from my clang-format version (I use v14) ? Please help me setting my environment with the expected formatter settings

This project uses Artistic Style to format code which is executed by make amalgamate or make pretty. You should disable clang-format when working on this code.

Thanks for taking a look at my PR! I am currently working on a cleaner iter_impl redesign to handle general case and (hopefully) fix MSVC compilation error. It complains because it does not like union with non trivial types (even if they are trivially copyable).

[nlohmann]

Please update to the latest develop branch (we renamed some folders in #3462).

[sjanel]

Please update to the latest develop branch (we renamed some folders in #3462).

Ok, done