Improve `node` and `node_view` interfaces about tomlplusplus HOT 20 CLOSED

@rbrugo Alrighty, it seems as though our ideas are pretty well aligned. I'll play around with some implementations tomorrow. Thanks again for your suggestions!

from tomlplusplus.

Hi, thanks for your fast reply.

RE: node.get_number()
I like the idea to make node::get_number() and node_view::value_or() template member functions. Perhaps get_number() template argument may be defaulted to double (since every int is representable as a double)?

RE: "make node_view more like optional" - has_value()
Maybe there could be a non-template has_value() which does what you said, and a template overload has_value<T>() which returns something like has_value() && std::is_same_v<T, viewed_type>() / std::is_convertible_to_v<viewed_type, T>()

RE: "make node_view more like optional" - value()
It's ok to have value() returning a std::optional. An alternative could be to have a throwing T & value<T>() and a std::optional<T> maybe_value<T>() noexcept, or opt_value<T>(), or something like this, what do you think?

from tomlplusplus.

Hi there! I'm really looking forward to see what you come up with here. I just spent today porting my game from cpptoml to this library (saw your post on reddit, got excited to work with an active library!).

I like what your doing with the library, but I second the suggestions above, working with pointers is really unergenomic compared to std::optional<> value_or(..) interface. I found myself writing a bunch of helper functions to convert the pointers into optional, which isn't great. I don't plan to leave it like that, but I thought I'd just mention it.

Thanks for making this library and pursuing this design feedback, and thank you rbrugo for proposing this suggestion.

edit: I just want to put this out there. I'm using the NO_EXCEPTIONS option, so I hope you remember us weirdos when you design a solution! 🤣

from tomlplusplus.

@bjadamson @rbrugo I've added a tentative 'fix' for this in the latest commit. Here's a summary:

node::value<T>(): ✔

If the node was not a value type (e.g. it was an array), or it was not the chosen value type, or it was not something that's sanely convertible to the chosen type, returns an empty std::optional<T>. Otherwise it will contain the value as the type you asked for, allowing for some sane conversions where appropriate (e.g. the value was actually an int, but you asked for a double). This should mean you don't need an explicit get_number() function, because that can be achieved via value<double>() etc.

node::value_or(): ✔

Same semantics as node::value_or(). Allows conversion between integers and floats. Allows retrieving strings as string views. node_view::value_or() has been updated to simply dispatch to the node's value_or(), so now also gets the same conversion semantics.

node_view::has_value(): ❌

I didn't add node_view::has_value() since that's what node_view::is_value() does already. I think the "is_" name prefix is better than "has_" here because the node_view is meant to act as a stand-in proxy for a node, rather than 'contain' one. Perhaps node_ref would have been a better name for the type...

Try it out and let me know what you think!

from tomlplusplus.

Would you mind sharing those helper functions you mentioned in a gist or something? To give me a clearer idea of the usage patterns people are likely to follow.

Sure thing! Here it is: https://gist.github.com/bjadamson/898392bb2ed27107817076a13701a319

Also, don't worry, your exception-less experience will stay that way :P

Amazing news!

@bjadamson @rbrugo I've added a tentative 'fix' for this in the latest commit.

Wow so quick! Awesome. I'll take a look at your commit as soon as I get a chance and let you know what I think. I don't have time to properly try it out right now. I just wanted to respond to your first comment so you could have that information.

from tomlplusplus.

Ah, I see, it was the assert() statements that were the meaningful part. Only problem with this design is that it runs counter to the current coercion/conversion behavior of value and value_or for integers and floats, so it either couldn't return by ref, or would have to not do the same as the other two functions, which would be inconsistent.

I could solve the inconsistency by naming the function something that makes it obvious that doesn't have the same semantics as the value functions and in fact returns a reference, e.g. ref(), then when combined with the fact that the assert used is already customizable using TOML_ASSERT, I could stick one in there.

// this
auto const z = get_floating_point_or_abort<float>("z", orb);
// could become this
auto const z = orb["z"].ref<double>(); // TOML_ASSERT() fired when this isn't sane

It might be better to choose a longer or scarier name, like ref_unchecked() or something. Open to bikeshedding here.

from tomlplusplus.

The semantics for that sort of thing are currently: it's illegal, and a static_assert would say as much. I'm trying as much as possible to enforce adherence to the core TOML types per the spec (64-bit signed ints, double-precision floats, etc.). I'd consider relaxing that for value and value_or, but ref() would have to be the correct types, since it's a reference to the actual data.

from tomlplusplus.

@bjadamson ref() is now a thing: https://marzer.github.io/tomlplusplus/classtoml_1_1node.html#a669d39909c44e25dccf8816a038ce4ef

from tomlplusplus.

Hey @rbrugo, thanks for the suggestions! You make good points, it would be nice to be able to do more with node directly when you're just extracting values from the config (which will be the most common use-case).

RE node.get_number()

Given that 'number' here means double or int64_t, what would the return type be? I suppose I could template it around the desired number type and have it cast-and-return if the underlying value type isn't the same. Should that then also be the behavior for node_view::value_or() when used with number types?

RE "make node_view more like optional"

value() would need to be templated on the value type, e.g. value<int64_t>(), which is pretty straightforward. What about has_value(), though? I'd expect that to be answering the question "does this node view reference a node, and is that node one of the value types?" (i.e. it wouldn't be templated like value<>()). How does that sound?

Also if value() is gonna throw like std::optional::value I need to decide how it should work when exceptions are disabled.

An alternative to all of the above of course is to just have value() return a std::optional.

from tomlplusplus.

@bjadamson Glad you like it! Would you mind sharing those helper functions you mentioned in a gist or something? To give me a clearer idea of the usage patterns people are likely to follow.

Also, don't worry, your exception-less experience will stay that way :P

from tomlplusplus.

@bjadamson Ah, thanks. It does seem as though the new functions will sufficiently replace your get_stringview() and get_floating_point().

Regarding get_array(): if you use the table class method get_as() instead of using operator[] to retrieve a node view, you can replace the whole thing with this:

toml::array const*
get_array(std::string_view const& sv, toml::table const& table)
{
  return table.get_as<toml::array>(sv);
}

(or, I guess because it's a one-liner, you could use table.get_as<toml::array>(sv) directly)

from tomlplusplus.

I had some time to try it out this morning, most of the suggestions you made panned out. However, I think I need to open a new issue for this, but would you consider an interface that supports my "or_abort" use case more directly?

Something like value_or_abort<T> ?

from tomlplusplus.

@bjadamson Ah, I didn't see those when I first looked at the gist. Perhaps I'm missing something obvious, but it's not entirely clear to me what they're actually doing. It looks like they just dereference the std::optional regardless of whether it contains a value or not. I guess that calls std::abort or something if you have exceptions disabled?

If it's a more monadic solution you're driving towards I've just added support for overriding the optional<T> used by the library so you can use one like https://github.com/TartanLlama/optional. In it's current form it requires exceptions (has throw statements unconditionally) but would be easy to modify.

from tomlplusplus.

Thanks for the library link, that looks interesting! Perhaps I picked a poor name when I said value_or_abort, but I'm actually looking for something simpler. Let me explain my use case / work flow, maybe that will be helpful. My workflow is to do a bunch of testing (with asserts on in debug mode) but in release mode have no runtime work done. Something with the following semantics:

1. returns whatever value I'm looking at by const&
   1. This is really the motivation for writing these helper functions, if I know already that the value isn't possibly empty, a function returning the value by const& is helpful (so I'm not having to deref the optional/pointer again).
2. In debug mode, it asserts that the value is there, but in release mode does nothing.
3. In debug mode, it asserts the type is what I am asking for, but in release mode does nothing.

I'm making this up by lets say something like

template <R>
R const& value_or_assert()
{
    assert(value());
    assert(value->is<R>());
    return *value_as<R>();
}

Here's my use case specifically, I'm loading the following entry from a TOML file

[[entity]]
name = "moon"
# ... other irrelevant components of the entity

[entity.orbital-body]
x = 1800.0
y = 1600.00
z = 1600.0

The orbital-body is an optional table within the [[entity]] array, however if the orbital-body is found, there MUST be an x, y, and z component (otherwise I want the program to assert immediately in debug mode)

auto const orbital_o = entity.get_as<toml::table>("orbital-body");
auto const texture_name_o = entity["texture"];

/// other code

if (orbital_o) {
      assert(texture_name_o); // There better be a texture for the moon
      assert(orbital_o->is_table());
      auto const& orb = *orbital_o->as_table();
      {
        // how the code is now, using helper functions
        auto const x = get_floating_point_or_abort<float>("x", orb);
        auto const y = get_floating_point_or_abort<float>("y", orb);
        auto const z = get_floating_point_or_abort<float>("z", orb);
        // ... whatever whatever

        // how I picture re-writing it with hypothetical function
        auto const x = orb["x"].value_or_assert<float>();
        auto const y = orb["y"].value_or_assert<float>();
        auto const z = orb["z"].value_or_assert<float>();

        // another possibility
        auto const x = orb["x"].value_or_assert();
        auto const y = orb["y"].value_or_assert();
        auto const z = orb["z"].value_or_assert();
      }

from tomlplusplus.

I like ref(), this is c++ it seems obvious that requesting a ref implies undefined behavior if the thing isn't actually what your asking for. I think some good documentation is sufficient.

from tomlplusplus.

Question though, what semantics are there if it's a double and I ask for float?

ie:

auto const z = orb["z"].ref<double>(); // correct
auto const z = orb["z"].ref<float>();  // does this do a conversion or fire an assert?
auto const z = orb["z"].ref<int>();   // ??

what about this:

[entity.health]
min = 32
max = 45

Then request them with:

auto const min = entity["min"].ref<int>(); // correct
auto const min = entity["min"].ref<float>(); // does this assert?
auto const min = entity["min"].ref<double>(); // does this assert?

from tomlplusplus.

Awesome, I like it.

from tomlplusplus.

This looks great! Thank you very much!

Question, the documentation shows a mutable reference. Is it intended to be able to mutate the value?

from tomlplusplus.

There's overloads matching the const and ref categories of the node. Yeah you'd be able to modify it directly using a non-cost ref, that's intentional.

from tomlplusplus.

@bjadamson An interface improvement for TOML_EXCEPTIONS=0 that might be relevant to your interests: in the latest release I've added support for getting node views and iterators directly from a toml::parse_result without needing to go via the table it contains (if any). Since node views and iterators can also be default-constructed and don't actually have to map to anything, this means you can skip error checking altogether in some circumstances:

toml::parse_result result = toml::parse_file("config.toml");
auto game_speed = result["speed"].value_or(1.0); // works OK even if parsing failed
for (auto&& [key, value] : result)
{
	// ... this loop will never run if parsing failed
}

(more info here)

from tomlplusplus.