Add implicit conversion operators from node to node_view

[Reedbeta]

Hello! I've made a couple of changes to loosen restrictions on the use of node_view. Primarily, I added implicit conversion operators from node to node_view (const and mutable variants). Also, I re-enabled the move assignment operator on node_view.

This was motivated by me running into trouble while trying to write code to traverse a table procedurally. Basically, I wanted to do something like this:

toml::node_view<const toml::node> child = myTable;
for (auto key : keyPath)
{
    child = child[key];
}

where I'm descending into a table using a list of keys in keyPath.

In opting to do this by adding implicit conversion operators (as opposed to e.g. adding a constructor on node_view, or explicit conversion functions), I was guided by std::string, which similarly has an implicit conversion to std::string_view. String views are always const, but here I wanted to allow either const or non-const node_views so I added both.

I also found that statements like child = child[key] above want the move assignment operator, which was disabled. I didn't see any reason for that to be disabled, especially considering that copy assignment is enabled, so I turned it back on.

I'm open to discussion on this if there are other ways you'd prefer to accomplish this, though. 🙂

Pre-merge checklist

• I've read CONTRIBUTING.md
• I've added new test cases to verify my change
• I've regenerated toml.hpp (how-to)
• I've updated any affected documentation (didn't do this as I don't have Doxygen installed)
• I've rebuilt and run the tests with at least one of:
  • Clang 6 or higher
  • GCC 7 or higher
  • Visual Studio 2019
• I've forever enshrined myself in glory by adding myself to the list of contributors in README.md

[marzer]

This looks good, though the move assignment operator being deleted was intentional, to solve the "misleading assignment problem" identified in this comment: #49 (comment)

Not really sure how else to get around that. I'm sure there's some egregious abuse of the C++ type system that could solve it better than my lazy workaround...

[marzer]

Re doxygen: you don't need to regenerate the documentation html itself since that's done by CI, but since you've expanded the public interface of node you'd need to add the appropriate doxygen markup to node.h for the conversion operators to appear in the output.

It's probably not really worth it here, though. That conversion seems so obvious I'm kinda annoyed at myself for not thinking of it to begin with :P

[marzer]

Hmm, now that I think about it more, the 'misleading assignment' issue could be 'solved' in the short-term just with some explicit caveat documentation. I'll add some next time I do a documentation pass.

That just leaves one thing before I can merge this: I happened to push a small cleanup commit at around the same time as your PR (like an hour earlier I think); can you re-base on it and regen toml.hpp again? Otherwise CI will probably fail when I merge.

[Reedbeta]

Sure, I don't mind adding a couple doc lines for the new conversions.

For the "misleading assignment" issue, I have a thought: what if the assignment operators were lvalue-only overloads? I think that ought to prevent temporary node_views from being assigned to. Not sure if that works, but I'll give it a try in a minute here.

[marzer]

I tried making them lvalue only and couldn't get it to compile, something about special members not being allowed to specify value category :(

[marzer]

You could probably get around that by deleting the copy-assignment and move-assignment operators, creating a proxy type that implicitly converts, and creating an lvalue assignment operator for that, though...

[Reedbeta]

Hmm, what was the error you got? I just tried the lvalue assignment operators now and it seemed to work, at least in MSVC. I'm on C++20 though, maybe that makes a difference...

[marzer]

I can't remember the exact text of it, but "special members" and "value category" were in it. I'm not at a PC currently so can't repro at the moment.

[Reedbeta]

It looks like it works on latest gcc, clang, and MSVC, in C++17 mode—just tested them all on godbolt. https://godbolt.org/z/zsGqra

[marzer]

Oh, well, that's good news. I was trying to explicitly delete the rvalue overloads, rather than explicitly default the lvalues, so I guess the semantics are a bit different (or I just fucked it up somehow, heh).

[Reedbeta]

Alright, I just pushed my updates, including rebasing onto your latest change - how does it look?

[marzer]

Looks great. Thanks @Reedbeta!

[whiterabbit963]

I think I ran into this, and just opted to use regular nodes, but this is a nicer solution.

Quick question though. Is there a reason to have

	TOML_EXTERNAL_LINKAGE
	node::operator node_view<node>() noexcept
	{
		return node_view<node>(this);
	}

since node_view is by definition const/read-only anyway?
It would appear that at best, it is a pointless/harmless distinction, and at worst, it is slightly misleading, but I thought I would point it out.

[Reedbeta]

You can still use the node() method or the various as() methods to get a pointer to the underlying node, and that could be const or non-const.

…

On Sat, Aug 8, 2020, 18:41 whiterabbit963 ***@***.***> wrote: I think I ran into this, and just opted to use regular nodes, but this is a nicer solution. Quick question though. Is there a reason to have TOML_EXTERNAL_LINKAGE node::operator node_view<node>() noexcept { return node_view<node>(this); } since node_view is by definition const/read-only anyway? It would appear that at best, it is a pointless/harmless distinction, and at worst, it is slightly misleading, but I thought I would point it out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#52 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMBN2XV6RXWHKO3KNAHTNDR7X5EPANCNFSM4PYZFGLQ> .

[whiterabbit963]

Except that the node_view api has all of those accessor methods returned as const. If node_view is modeled after string_view, that seems to be the "right" thing to do. In addition, if you were able to get a non-const reference to the underlying node, would that be a bug?

[Reedbeta]

the node_view api has all of those accessor methods returned as const

No, it preserves the constness of the node it's viewing. A node_view<const node> returns everything as const, but a node_view<node> doesn't.

if you were able to get a non-const reference to the underlying node, would that be a bug?

Why would that be a bug?

[whiterabbit963]

My understanding of view's is that the underlying data is intended to be immutable. The string_view class appears to uphold this intent. The node_view api also appears to uphold this intent as all of the class member functions are marked const.

If by definition a view is only allowed to access the underlying data as const, then there is effectively no difference between node_view<node> and node_view<const node>, as you should not be allowed to modify the node from the view. If you did want to allow mutability to the underlying node, I have to wonder what the design purpose is behind a node_view, when a node has such a similar API.

You could make a case for returning a non-const node_view, if there were methods that changed how the underlying node was viewed. For example, the string_view has a method remove_prefix(), that provides a way to modify the view of the underlying string, but I do not see how this would translate to a view of a node. I also do not see any methods in the API that would apply.

Is there a code sample that wouldn't compile if the following wasn't implemented?

	TOML_EXTERNAL_LINKAGE
	node::operator node_view<node>() noexcept
	{
		return node_view<node>(this);
	}

[marzer]

the node_view api has all of those accessor methods returned as const

The accessor methods don't modify the state of the view (i.e. they never re-bind it to a different node), so they're all const. The const-ness of the node itself is propagated via the type parameter in the node_view template itself, and is determined by the const-ness of wherever you got the initial view from (e.g. table::operator[] on a const table gives a node view of a const node, and any sub-views created from that will be of const nodes too).

My understanding of view's is that the underlying data is intended to be immutable

No, the data is mutable (if it's a not a node_view<const node>), but the type of it isn't; you can change an int value through a node view of a value<int64_t>, add an element to an array through a node view of an array, etc, but you can't re-assign a value<std::string> node as a table or something.

Maybe node_view wasnt the best name for it, but I've made that bed now...

[marzer]

To further illustrate the distinction @whiterabbit963, have a look at the example at the top of the node_view API docs page. Specifically, this line:

tbl["products"][0]["keywords"].as_array()->push_back("heavy");

tbl is non-const, so the first node_view retrieved by tbl["products"] is a node_view<node>, as are the two sub-views created by [0]["keywords"]. This means that the call to as_array() returns array*, not const array*, thus making a call to push_back() possible.

OTOH if tbl were const, tbl["products"] would return a node_view<const node>, as would the sub-views, et cetera.

[marzer]

If node_view is modeled after string_view

It is in name only. It's more like a std::optional<node&>, if such a thing were legal. In a pre-release version of the library it was called node_ref, but that's not a perfect fit either. Naming stuff is hard, heh.

[whiterabbit963]

Ah, ok, the underlying type is wrapped by a newly constructed non-const object. The example really helped. Thanks for the explanation!

Effectively, you have expanded upon std::optional<std::reference_wrapper<node>>, which is the STL's solution to the issue.

Naming it node_wrapper feels the most accurate, if a little verbose. Agreed, naming stuff is not easy.

[marzer]

@Reedbeta I have some unfortunate news; I've had to make these operators explicit- while working on a side project I discovered the implicit conversions were causing some problematic interplay with node_view::operator== and friends.

To counter the inconvenience I've added additional constructors for node_view so you can make one from a reference or pointer to any node, as well as deduction guides to reduce the syntactic burden:

toml::table tbl = toml::parse("foo.toml");
const auto& ctbl = tbl;
auto ptbl = &tbl;
auto cptbl = &ptbl;

auto nv   = node_view{ tbl }; // node_view<node>
auto cnv  = node_view{ ctbl }; // node_view<const node>
auto nv2  = node_view{ ptbl }; // node_view<node>
auto cnv2 = node_view{ cptbl }; // node_view<const node>

[Reedbeta]

Ahh, okay. That should be fine for me, thanks for the heads-up.

I suspect the (explicit) conversion operators are a bit redundant now that the new constructors exist? I'm not sure there's any case the conversions would be useful where you couldn't use the constructors as well. In any case, I can easily adjust the couple places in my code where I use them.

[marzer]

Yeah, they are definitely a bit redundant now. I considered removing them while I was there but then decided against it mainly because they aren't doing any harm. My gut feeling is that there's some specific language-lawyer-ish reason they might be useful, but I can't think of one myself.