search

dropbox / json11

A tiny JSON library for C++11.
MIT License
2.55k stars 613 forks source link

Language issue causes dangerous pattern for nested arrays #86

Closed artwyman closed 5 years ago

artwyman commented 7 years ago

Issue for tracking a long-standing issue which was never tracked publicly before. It's natural to want to construct a nested JSON array as follows: Json nested_array = Json::array { Json::array { 1, 2, 3 } }; This used to work as expected, until a language standard change DR1467 which caused the array constructor to be interpreted as a move instead of creating a nested array from an initializer_list. This is only an issue for single-element arrays, since 2 elements would break the ambiguity and be interpreted as an initializer_list.

Most new compilers have the new dangerous behavior now. There's a "canary" in the unit test which is meant to point out whether your compiler has the bad behavior: https://github.com/dropbox/json11/blob/master/test.cpp#L195

There is a new issue being tracked by the standards committee on this, but I haven't seen any action. Latest status is here: http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2137

Meanwhile the best workaround is to build these nested arrays in multiple steps using push_back()

artwyman commented 7 years ago

I'm going to disable the canary in the test case by default, since it doesn't look like the standards committee is going to address this anytime soon. It seems more important that new users of the library can make test and run successfully the first time without being confused.

timsong-cpp commented 7 years ago

Core issue 2137 was resolved by P0384R0 back in June 2016. The core issues list is just (very) slow to update.

artwyman commented 7 years ago

@timsong-cpp thanks for the update. I'm not familiar enough with terminology of the standard or the operations of the committee to be sure what that document is saying. Can you provide any color? There's a "proposed resolution" from April in a doc of "tentatively ready" issues from June. Does that mean the proposed resolution was was accepted? What does the proposed text actually mean for the example?

I'm hopeful that this might actually be a useful resolution, in which case we then only need to wait for compilers to implement the change.

timsong-cpp commented 7 years ago

There's a "proposed resolution" from April in a doc of "tentatively ready" issues from June. Does that mean the proposed resolution was was accepted?

It was accepted. (The core - and library - working group prepares lists of ready and tentatively ready issues for each meeting for adoption by the full committee. Almost all of the time they all get adopted - and this one was, but to be sure in the general case you'd need to check a few more places.)

What does the proposed text actually mean for the example?

It reverts the DR1467 change for non-aggregate class types. Those will get the previous behavior.

artwyman commented 7 years ago

Ah, I didn't realize that change was a revert of the previous one, and I just educated myself on the formal definition of aggregate class, so this sounds good for json11's needs. Looking forward to seeing it implemented. Thanks.

artwyman commented 5 years ago

I'm pretty sure the "right" language behavior is now in all the recent compiler versions, so I'm closing this issue out.