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

Implement support for string_view (attempt no. 3) #3423

Merged
merged 5 commits into from
Apr 29, 2022

Conversation

falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger commented Apr 5, 2022

Based heavily on the previous 2 attempts of @nlohmann with some cleanups of mine.

Closes #1529.
Closes #2685.
Closes #3237.

Note: Depends on #3415 and needs to be rebased. Done.


Add key_compare member to ordered_map

Replace == with key_compare in ordered_map

Expose the actual comparison function used by object_t

nlohmann::ordered_map uses a different comparison function than the one provided via template parameter.

  • Introduce a type trait to detect if object_t has a key_compare member.
  • Rename object_comparator_t to default_object_comparator_t.
  • Add object_comparator_t to be conditionally defined as object_t::key_compare, if available, or default_object_comparator_t otherwise.
  • Update the documentation accordingly.

Add type traits to check if a type is usable as object key

Add type trait to check:

  • if a type is a specialization of a template.
  • if a type is a json_pointer.
  • if a type is a basic_json::{const_,}iterator.
  • if two types are comparable using a given comparison functor.
  • if a type is comparable to basic_json::object_t::key_type.
  • if a type is usable as object key.

Based on previous work by @nlohmann.

Rework basic_json element access to accept more key types

Rework basic_json element access member functions and operators to accept any type that meets the requirements defined by type trait detail::is_usable_as_key_type.

Member functions and operators:

  • at()
  • operator[]
  • value()
  • erase()
  • find()
  • count()
  • contains()

Update documentation to reflect these changes.

Add unit tests to excercise the new functions using std::string_view.

Based on previous work by @nlohmann.

@coveralls
Copy link

coveralls commented Apr 5, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling 078673f on falbrechtskirchinger:string_view3 into a6ee8bf on nlohmann:develop.

@nlohmann nlohmann added the please rebase Please rebase your branch to origin/develop label Apr 12, 2022
@falbrechtskirchinger falbrechtskirchinger marked this pull request as ready for review April 12, 2022 15:10
@nlohmann nlohmann removed the please rebase Please rebase your branch to origin/develop label Apr 12, 2022
doc/mkdocs/docs/api/basic_json/at.md Outdated Show resolved Hide resolved
doc/mkdocs/docs/api/basic_json/contains.md Outdated Show resolved Hide resolved
doc/mkdocs/docs/api/basic_json/count.md Outdated Show resolved Hide resolved
doc/mkdocs/docs/api/basic_json/value.md Outdated Show resolved Hide resolved
doc/mkdocs/docs/api/ordered_map.md Outdated Show resolved Hide resolved
include/nlohmann/detail/meta/type_traits.hpp Show resolved Hide resolved
include/nlohmann/json.hpp Show resolved Hide resolved
doc/mkdocs/docs/api/basic_json/find.md Outdated Show resolved Hide resolved
@falbrechtskirchinger
Copy link
Contributor Author

Downgrading back to draft status. I made substantial changes and want to see if they pass CI. Will update the doc next and re-request review when it's ready.

@falbrechtskirchinger falbrechtskirchinger force-pushed the string_view3 branch 3 times, most recently from 1632077 to fb8a93f Compare April 13, 2022 20:23
@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Apr 14, 2022

@nlohmann I'm trying something new and wanted to get your feedback before I continue.

I've added the mkdocs-macros-plugin and have defined re-used phrases in a separate file that is included where needed. I'm testing these changes in at.md and contains.md.

Example:

3. See 2. {{ snippets.keytype_is_transparent }}

translates to:

3. See 2. This overload is only available if `KeyType` is comparable with `#!cpp typename object_t::key_type` and `#!cpp typename object_comparator_t::is_transparent` denotes a type.

Essentially, if we want to tweak the wording we can do so in one place now.

Shall I continue or do you want me to roll that back?

@nlohmann
Copy link
Owner

@nlohmann I'm trying something new and wanted to get your feedback before I continue.

I've added the mkdocs-macros-plugin and have defined re-used phrases in a separate file that is included where needed. I'm testing these changes in at.md and contains.md.

Example:

3. See 2. {{ snippets.keytype_is_transparent }}

translates to:

3. See 2. This overload is only available if `KeyType` is comparable with `#!cpp typename object_t::key_type` and `#!cpp typename object_comparator_t::is_transparent` denotes a type.

Essentially, if we want to tweak the wording we can do so in one place now.

Shall I continue or do want me to roll that back?

I'd rather have it explicit, because this makes it easier for others to edit the Markdown files. I am aware of the DRY violation etc., but I'm afraid this will add too much complexity.

@falbrechtskirchinger falbrechtskirchinger force-pushed the string_view3 branch 2 times, most recently from cac3cc1 to ba13de3 Compare April 15, 2022 17:23
@Ericson2314
Copy link

Would be very nice if this could happen!

@falbrechtskirchinger falbrechtskirchinger marked this pull request as ready for review April 23, 2022 14:43
doc/mkdocs/docs/api/basic_json/value.md Outdated Show resolved Hide resolved
doc/mkdocs/docs/api/basic_json/erase.md Outdated Show resolved Hide resolved
include/nlohmann/detail/macro_scope.hpp Show resolved Hide resolved
@falbrechtskirchinger falbrechtskirchinger force-pushed the string_view3 branch 2 times, most recently from 59d8fd5 to d659ed9 Compare April 25, 2022 14:00
@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Apr 25, 2022

I've discovered some last minute issues yesterday that were bad enough to make me question the overall soundness of this PR. After some careful tweaks I am now cautiously optimistic that everything has been resolved (pending green CI and 100% coverage).
A few more overloads were necessary for the API to match the documented behavior.

I plan on reviewing everything once again side-by-side with the documentation to make sure they match up.

Edit: CI is green and coverage is back to 100%. Looks promising. :-)

doc/mkdocs/docs/api/basic_json/operator[].md Outdated Show resolved Hide resolved
doc/mkdocs/docs/api/basic_json/operator[].md Outdated Show resolved Hide resolved
doc/mkdocs/docs/api/basic_json/erase.md Outdated Show resolved Hide resolved
doc/mkdocs/docs/api/basic_json/contains.md Outdated Show resolved Hide resolved
doc/mkdocs/docs/api/basic_json/at.md Outdated Show resolved Hide resolved
doc/mkdocs/docs/api/basic_json/at.md Outdated Show resolved Hide resolved
falbrechtskirchinger and others added 4 commits April 25, 2022 22:45
nlohmann::ordered_map uses a different comparison function than the one
provided via template parameter.
* Introduce a type trait to detect if object_t has a key_compare member.
* Rename object_comparator_t to default_object_comparator_t.
* Add object_comparator_t to be conditionally defined as
  object_t::key_compare, if available, or default_object_comparator_t
  otherwise.
* Update the documentation accordingly.

Co-authored-by: Niels Lohmann <[email protected]>
Add type trait to check:
* if a type is a specialization of a template.
* if a type is a json_pointer.
* if a type is a basic_json::{const_,}iterator.
* if two types are comparable using a given comparison functor.
* if a type is comparable to basic_json::object_t::key_type.
* if a type has a member type is_transparent.
* if a type is usable as object key.
* if a type has an erase() function accepting a given KeyType.

Co-authored-by: Niels Lohmann <[email protected]>
Rework basic_json element access member functions and operators to
accept any type that meets the requirements defined by type trait
detail::is_usable_as_key_type.

Member functions and operators:
* at()
* operator[]
* value()
* erase()
* find()
* count()
* contains()

Update documentation to reflect these changes.

Add unit tests to excercise the new functions using std::string_view.

Co-authored-by: Niels Lohmann <[email protected]>
@falbrechtskirchinger
Copy link
Contributor Author

I've gone through the documentation and the code side-by-side to make sure that all documented functions are in fact there and correct. I've also added a comment to a particularly unrecognizable overload:

    // this is the value(const typename object_t::key_type&) overload
    template < class KeyType, class ValueType, detail::enable_if_t <
                   std::is_same<KeyType, typename object_t::key_type>::value
                   && detail::is_getable<basic_json_t, ValueType>::value
                   && !std::is_same<value_t, ValueType>::value, int > = 0 >
    typename std::decay<ValueType>::type value(const KeyType& key, ValueType && default_value) const
    {

I believe this thing is now done and ready for final reviews.

My suggestion would be to merge #3336 first, though. I'll have an easier time resolving merge conflicts, if there are any.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann self-assigned this Apr 29, 2022
@nlohmann nlohmann added this to the Release 3.11.0 milestone Apr 29, 2022
@nlohmann nlohmann merged commit 5352856 into nlohmann:develop Apr 29, 2022
@nlohmann
Copy link
Owner

Thanks a lot! This is really a milestone!!!

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

Successfully merging this pull request may close these issues.

How can I use std::string_view as the json_key to "operator []" ?
5 participants