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

array insertion fails const iterator conversion #32

Closed
ned14 opened this issue May 22, 2020 · 4 comments
Closed

array insertion fails const iterator conversion #32

ned14 opened this issue May 22, 2020 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@ned14
Copy link

ned14 commented May 22, 2020

toml::array to, from;
for(size_t n = 0; n < from.size(); n++)
{
  to.insert(to.end(), std::move(from[n]));  // fails with cannot convert argument 1 from 'toml::array::iterator' to 'toml::array::const_iterator'
}
@ned14 ned14 added the bug Something isn't working label May 22, 2020
@ned14 ned14 changed the title table insertion fails const iterator conversion array insertion fails const iterator conversion May 22, 2020
@marzer
Copy link
Owner

marzer commented May 23, 2020

I've implemented a fix for the iterator conversion though in doing so I've realized that what you're trying to do in this snippet won't work anyway; array::operator[] returns a toml::node&, and since toml::node is an abstract base, what you're trying to do wouldn't work without access to the underlying std::unique_ptr<node>. I could make it work as a copy, but it couldn't be a move.

I don't really have a good alternative without restructuring the node<=>table/array/value relationships to rely on something other than inheritance (as mentioned in #31). I'd really like to avoid introducing a 'moved from' or invalid state to the node objects (currently if a node exists it is a valid something), but I can implement something like std::list's splice() for merging arrays as a short-term solution.

I could also provide some means for working with the underlying std::vector<std::unique_ptr<node>> directly. That might actually turn out to be the best option; the simple public interface of the arrays and tables can stay more-or-less how it is (optimized for people largely just interested in querying the data), and people needing to do more advanced stuff can work with the underlying node data directly.

@marzer
Copy link
Owner

marzer commented May 23, 2020

The iterator conversion fix is live in 289c95c, though it doesn't solve the underlying issue I discuss above. I'll leave this issue open and await your ideas on a path forward, since my fix of the issue doesn't really get at the heart of it just yet.

@ned14
Copy link
Author

ned14 commented May 25, 2020

In my code I was actually visiting the node before moving it. The code above was for exposition only. Incidentally, you currently have visit() as a member function, I believe it ought to be an ADL discovered free function overload. See https://en.cppreference.com/w/cpp/utility/variant/visit for the API.

I'm going to consider this closed. Thanks for the rapid fix!

@ned14 ned14 closed this as completed May 25, 2020
@marzer
Copy link
Owner

marzer commented May 25, 2020

RE visit: toml++'s visit has slightly different semantics to std::visit in that it allows for the visitor to be non-exhaustive; it doesn't have to be callable for every possible node specialization, only the ones you need, so it's not really intended to be compatible with/related to std::visit in any way (I can't think of a situation where code would be so generic that std::visit compatibility would even make sense).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants