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

JSON as_or accessor #591

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

JSON as_or accessor #591

wants to merge 2 commits into from

Conversation

supsm
Copy link

@supsm supsm commented Feb 9, 2025

Implements feature request from #589.

Some caveats/things to consider:

  • Numbers will only return the held value (i.e. not the default value) if it matches exactly. Floating-point and integer types are not interchangeable (and string numbers also don't get parsed). It's probably possible to change this, but I'm not sure what the expected behavior should be in this case.
  • get_value_or sort of has an ambiguous name: it isn't clear that it is getting the value of a member as opposed to the value of the current object (which as_or does)
  • as_or should probably be marked noexcept, but I'm not entirely sure whether something else could throw.

Other things I've noticed:

  • in as (basic_json.hpp:3288), it is implemented as
T val = json_type_traits<...>::as(*this);
return val;

instead of returning from json_type_traits::as() directly. I've copied this behavior for as_or, but I'm wondering if this is something intentional.

  • Some of the tests are inconsistent with each other. In particular, the "test proxy get_value_or" test is quite different in structure and format from the "get_value_or test"

@danielaparker
Copy link
Owner

danielaparker commented Feb 11, 2025

I appreciate the work. But the points you raise do suggest concerns. I need to think about this.

Regarding get_value_or, I have to confess that I've never been enthusiastic about the name. nlohmann has a similar function that they call value, but that name isn't inspiring either. At one time I thought of renaming it to at_as_or, which I think is suggestive, but my brain said no! In retrospect I think I prefer the lighter notation get_or, get is commonly used in other programming languages for getting a value by name from a map. The _or only applies here if the value isn't found.

is<T>() and as<T>() were contributed very early in this project's life, in PR #3 (2014). is<T>() has always been strict, if is<T>() passes, it's generally safe to call as<T>(). is<T>() doesn't guarantee that as<T>() won't throw, however, as as<T>() may need to allocate memory. as<T>() on its own is generally permissive and coerces to the requested type if sensible, there's some "user beware" here.

@danielaparker
Copy link
Owner

As you've noted in your comments, as currently implemented,

j.as<T>()

and

j.as_or<T>(<default_value>)

may return different values for some values where j.as<T>() doesn't throw. I think that's undesirable (and hard to explain in the documentation.)

An alternative implementation, rather than doing the check first with j.is<T>(), would be to attempt as<T>() and return default_value only if the attempt failed.

A simple implementation might be

            using T_type_traits = json_type_traits<basic_json,T>;
            JSONCONS_TRY
            {
                T val = T_type_traits::as(*this);
                return val;
            }
            JSONCONS_CATCH(...)
            {
                return static_cast<T>(std::forward<U>(default_value));
            }

Relying on try/catch isn't ideal, though. We have users that don't want exceptions, and define JSONCONS_NO_EXCEPTIONS to turn throws into std::terminate.

It would be helpful if json_type_traits<basic_json,T>::as() reported errors with error codes, rather than throwing. I've been thinking about that, and it is doable in a backwards compatible way.

@supsm
Copy link
Author

supsm commented Feb 11, 2025

I definitely agree that try/catch isn't a great solution. Even for users who have exceptions enabled, the performance impact could be significant (especially since such a function isn't typically expected to be expensive).

Other options including applying is_number instead of is<T> if T is a numeric type, or adding a separate non-strict is<T> to json_type_traits. These may easier or find use in other contexts, but their performance will probably be slightly worse than an as with an error code due to double checking.

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

Successfully merging this pull request may close these issues.

2 participants