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.

Sign up for GitHub

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

Jump to bottom

Remove undefined behavior for const operator[] #2111

Closed
Xylaant opened this issue May 15, 2020 · 6 comments
Closed

Remove undefined behavior for const operator[] #2111

Xylaant opened this issue May 15, 2020 · 6 comments
Labels
kind: enhancement/improvement state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Comments

@Xylaant
Copy link

Xylaant commented May 15, 2020

  • Describe the feature in as much detail as possible.

In most places in the library, when using a non-const object, attempting to access a non-existent array value or key via the operator[] methods will result in creation of those methods. On the other hand, if the basic_json object is const qualified, then for arrays operator[] results in undefined behavior, where for objects it acts more like the at() function with an exception.

I'd like to propose modifying the implementations of the const overloads of operator[] to instead return a const-reference to a static instance of an empty basic_json<> object. There are a handful of ways to do this (static data member or static singleton method). This would mean that you can't easily slip into undefined behavior/exception by adding const qualifying a json object.
@nlohmann
Copy link
Owner

A lot of people were annoyed by the behavior of operator[], but this is the first time someone actually has a constructive proposal how to improve the situation. I will consider this after the 3.8.0 release.

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label May 15, 2020
@Xylaant
Copy link
Author

Xylaant commented May 16, 2020

As part of the discussion, what do you think the correct behavior of operator const and operator const be if is_null() is true. For the non-const version, it converts the object to an array or object.

If the code were to return a constant reference to a nil object in the not-present case, should it also do that in the is_null() case? The current behavior would be to throw an exception. Is it worth considering have it just return the same null object?

@stale
Copy link

stale bot commented Jun 17, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jun 17, 2020
@stale stale bot closed this as completed Jun 26, 2020
@nlohmann nlohmann reopened this Jul 25, 2020
@stale stale bot removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jul 25, 2020
@eferreira
Copy link

A lot of people were annoyed by the behavior of operator[], but this is the first time someone actually has a constructive proposal how to improve the situation. I will consider this after the 3.8.0 release.

For the record, I suggested this exact possibility in this thread back in 2017: #289 (comment)

would still love for it to make it into the official library in some form or another, for all the same reasons that I went into back then.

@gregmarr gregmarr mentioned this issue Sep 9, 2020
Closed
3 tasks
@stale
Copy link

stale bot commented Oct 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Oct 4, 2020
@stale stale bot closed this as completed Oct 12, 2020
@TLescoatTFX
Copy link

Hello,

It is a bit painful to have to check every key, especially when the researched value is deep. This proposal (just returning some kind of sentinel) would allow to search deep values in a user-friendly way , ex:

const json& val = some_json["data"]["key"]["another_key"]["slot"];
if(!val.is_null()) // ...

You mentioned you would consider this after the 3.8.0. Do you have some opinion on this request ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind: enhancement/improvement state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nlohmann @eferreira @Xylaant @TLescoatTFX