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.

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

Indexing const basic_json<> with const basic_string<char> #157

Closed
gnzlbg opened this issue Dec 15, 2015 · 7 comments
Closed

Indexing const basic_json<> with const basic_string<char> #157

gnzlbg opened this issue Dec 15, 2015 · 7 comments

Comments

@gnzlbg
Copy link

gnzlbg commented Dec 15, 2015

I cannot index const basic_json<> with a const basic_string<char> anymore.

In file included from my/json.hpp.cpp:2:
my/json.hpp:30:11: error: no viable overloaded operator[] for type 'const json' (aka 'const basic_json<>')
  return j[name];
         ~^~~~~
src/json.hpp:2593:15: note: candidate function not viable: 'this' argument has type 'const json' (aka 'const basic_json<>'), but method is not marked const
    reference operator[](size_type idx)
              ^
src/json.hpp:2634:21: note: candidate function not viable: no known conversion from 'const string' (aka 'const basic_string<char>') to 'size_type' (aka 'unsigned long') for 1st argument
    const_reference operator[](size_type idx) const
                    ^
src/json.hpp:2671:15: note: candidate function not viable: 'this' argument has type 'const json' (aka 'const basic_json<>'), but method is not marked const
    reference operator[](const typename object_t::key_type& key)
              ^
src/json.hpp:2718:15: note: candidate template ignored: could not match 'const T [n]' against 'const string' (aka 'const basic_string<char>')
    reference operator[](const T (&key)[n])
@gnzlbg
Copy link
Author

gnzlbg commented Dec 15, 2015

OK it seems that fixing #135 caused this.

Right now I have a lot of code that passes const basic_json<> objects around, asserts on debug mode that the elements exists, and uses operator[] const to access the elements. Most of the assertions are coalesced, e.g. on object construction, and before loops. In release mode nothing is checked.

The "workaround" proposed in #135 is to use at which performs the checks on every access. This is not good enough for me since I know that the elements are there when I use operator[] const.

Basically what I need for my use case is:

unchecked const access with undefined behavior if the element is not there.

I would have prefered if, instead of removing operator [] const, the NDEBUG macro would have been used to assert or throw if the element is not there.

Sadly I didn't knew about #135 untill all my code just broke (nothing serious, I am just checking a previous version of the library now).

Maybe next time such an API breaking change is being discussed the [[deprecated]] macro can be used instead for a couple of weeks. That produces a warning instead of breaking the code, and allows those interested to chime into the discussion.

@nlohmann
Copy link
Owner

Hi @gnzlbg, sorry for the inconvenience. As there are no proper releases yet, I cannot really deprecate functionality, because everything in the master branch is basically just a development snapshot. I hope to have a "proper" 1.0 done soon and finally get some roadmap. Then, deprecating functions can be properly documented.

As for #135, I though quite a while, but better similarity to STL was the reason to change the API. Once again, sorry for your troubles.

@gnzlbg
Copy link
Author

gnzlbg commented Dec 15, 2015

No worries! I understand the motivation to do as the STL does and can live with an older version. If a method for unchecked const access is ever added please cite this issue so that I notice it.

@nlohmann
Copy link
Owner

If you are sure the key (let's say mykey) exists in your object, why don't use *j.find("mykey")?

@gnzlbg
Copy link
Author

gnzlbg commented Dec 15, 2015

This is one of the things i'm doing in a loop (i is an index):

... j["files"][i]]["fields"][name];

Using at adds 4 unnecessary checks and looks like this (which is not that
bad):

... j.at("files").at(i).at("fields").at(name);

Using find it looks like:

... *(*(*(*j.find("files")).find(i)).find("fields")).find(name);

@gregmarr
Copy link
Contributor

iterator also has operator->
j.find("files")->find(i)->find("fields")->find(name);

@gnzlbg
Copy link
Author

gnzlbg commented Dec 15, 2015

That should work, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants