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

Re-template json_pointer on string type #3415

Merged
merged 11 commits into from
Apr 12, 2022

Conversation

falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger commented Apr 3, 2022

This PR changes the json_pointer template parameter from basic_json to string type and deals with unexpected complications.

Most notably, a helper function for string concatenation is introduced to allow for some flexibility in the implementation of string types (and eliminate some temporaries as a positive side-effect). I'll leave it to others to remove more std::string API dependence (some low-hanging fruits are empty(), find_first_of(), substr()).

To Do:

  • Remove context parameter default value as per greg's suggestion in the previous PR.
  • Add backwards compatibility with json_pointer<basic_json>.
  • Update documentation.
  • Fix issue references in commits once ready for merge. (Don't want to spam GH issues as much.)

@coveralls
Copy link

coveralls commented Apr 4, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling fdb15f9 on falbrechtskirchinger:refactor-json_ptr-tpl into 261cc4e on nlohmann:develop.

@falbrechtskirchinger
Copy link
Contributor Author

@nlohmann Can you please take a look at the Codacy report and tell me which tool is generating that strlen warning? This link is not accessible to me: https://app.codacy.com/gh/nlohmann/json/patterns/list?selectedEngine=81806a42-1d70-40e6-ad07-9a1a9da9e500#pattern-6542

include/nlohmann/json.hpp Outdated Show resolved Hide resolved
include/nlohmann/detail/string_concat.hpp Show resolved Hide resolved
@nlohmann
Copy link
Owner

nlohmann commented Apr 4, 2022

@nlohmann Can you please take a look at the Codacy report and tell me which tool is generating that strlen warning? This link is not accessible to me: https://app.codacy.com/gh/nlohmann/json/patterns/list?selectedEngine=81806a42-1d70-40e6-ad07-9a1a9da9e500#pattern-6542

Sure:
image

image

Looks like a false positive - I ignored the issue. This should become effective in the next check.

@falbrechtskirchinger
Copy link
Contributor Author

@nlohmann Can you please take a look at the Codacy report and tell me which tool is generating that strlen warning? This link is not accessible to me: https://app.codacy.com/gh/nlohmann/json/patterns/list?selectedEngine=81806a42-1d70-40e6-ad07-9a1a9da9e500#pattern-6542

(...)
Looks like a false positive - I ignored the issue. This should become effective in the next check.

Okay, thanks, I would have added // Flawfinder: ignore now that I know the tool.

@gregmarr
Copy link
Contributor

gregmarr commented Apr 4, 2022

Just a thought, instead of const char *cstr, could this be template<int N> ... const char (&cstr)[N] so that we avoid the strlen altogether?

@falbrechtskirchinger
Copy link
Contributor Author

Just a thought, instead of const char *cstr, could this be template<int N> ... const char (&cstr)[N] so that we avoid the strlen altogether?

We'd still need const char * for things like type_name() which returns const char * (*) and then one'd have to handle the ambiguity between the two, either by introducing a helper type with template specialization for both cases or using enable_if/SFINAE.
I considered it but it's not worth the hassle in my opinion.

(*) I mean, we don't have to but it's nice to avoid std::string(type_name()).

@falbrechtskirchinger falbrechtskirchinger marked this pull request as ready for review April 5, 2022 04:42
@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Apr 5, 2022

Once this passes review, prior to merge, I'd just like to do one last force-push to update commit messages with the issue references.

Edit: Forgot to include json_pointer/string_t.md. Fixed.

@gregmarr
Copy link
Contributor

gregmarr commented Apr 5, 2022

We'd still need const char * for things like type_name()

Agreed that it only makes sense if the only things are string literals and string objects, but doesn't make sense if there are true raw pointers.

Copy link
Contributor Author

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

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

I discovered a few minor things in my full review. Will fix momentarily.

include/nlohmann/detail/conversions/from_json.hpp Outdated Show resolved Hide resolved
include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved
include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved
include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved
include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved
include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved
include/nlohmann/detail/string_concat.hpp Outdated Show resolved Hide resolved
include/nlohmann/json.hpp Show resolved Hide resolved
test/src/unit-alt-string.cpp Show resolved Hide resolved
test/src/unit-regression2.cpp Show resolved Hide resolved
Copy link
Contributor Author

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

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

Ready for final review and merge.

doc/mkdocs/docs/api/json_pointer/index.md Show resolved Hide resolved
include/nlohmann/detail/meta/type_traits.hpp Show resolved Hide resolved
include/nlohmann/json.hpp Outdated Show resolved Hide resolved
include/nlohmann/json.hpp Outdated Show resolved Hide resolved
include/nlohmann/json.hpp Outdated Show resolved Hide resolved
include/nlohmann/json.hpp Outdated Show resolved Hide resolved
include/nlohmann/json.hpp Outdated Show resolved Hide resolved
include/nlohmann/json.hpp Outdated Show resolved Hide resolved
doc/mkdocs/docs/api/json_pointer/string_t.md Outdated Show resolved Hide resolved
include/nlohmann/detail/json_pointer.hpp Outdated Show resolved Hide resolved
include/nlohmann/detail/json_pointer.hpp Show resolved Hide resolved
include/nlohmann/detail/json_pointer.hpp Show resolved Hide resolved
include/nlohmann/detail/json_pointer.hpp Show resolved Hide resolved
include/nlohmann/detail/json_pointer.hpp Show resolved Hide resolved
include/nlohmann/json.hpp Outdated Show resolved Hide resolved
Change exception context parameter to pointer and replace context with
nullptr where appropriate.
@falbrechtskirchinger
Copy link
Contributor Author

@nlohmann 3.10.6 is now 3.11.0, right? So I need to update the version numbers?

@nlohmann
Copy link
Owner

nlohmann commented Apr 8, 2022

@nlohmann 3.10.6 is now 3.11.0, right? So I need to update the version numbers?

Yes, so much has piled up. But I would have gone throw the release anyway to clean this up.

@falbrechtskirchinger
Copy link
Contributor Author

You probably missed my comment on this closed PR: #3418 (comment)
Interested?

@nlohmann
Copy link
Owner

nlohmann commented Apr 8, 2022

You probably missed my comment on this closed PR: #3418 (comment)
Interested?

Not right now.

@falbrechtskirchinger
Copy link
Contributor Author

Version numbers updated. Mixed in some accidental edits that were supposed to go into a different branch and fixed that too.

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 12, 2022
@nlohmann nlohmann added this to the Release 3.11.0 milestone Apr 12, 2022
@nlohmann nlohmann merged commit 616caea into nlohmann:develop Apr 12, 2022
@nlohmann
Copy link
Owner

Thanks!

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.

4 participants