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

obj.erase(it) fails on map #19

Open
vinipsmaker2 opened this issue Feb 8, 2018 · 8 comments
Open

obj.erase(it) fails on map #19

vinipsmaker2 opened this issue Feb 8, 2018 · 8 comments

Comments

@vinipsmaker2
Copy link
Contributor

Tested on commit cea147e, but looking at the source code from latest develop, I think it should be failing too. I can write an isolated case to test it on develop later.

Compiler is clang. Error is:

application.cpp:36:9: error: no matching member function for call to 'erase'
    obj.erase(it);
    ~~~~^~~~~
3rd/trial.protocol/include/trial/dynamic/variable.hpp:869:14: note: candidate function not viable: no known conversion
      from 'trial::dynamic::basic_variable<std::allocator>::key_iterator' to 'trial::dynamic::basic_variable<std::allocator>::const_iterator' for 1st argument
    iterator erase(const_iterator position);
             ^

Also, finally I am coding the part where performance is not the critical and I can do whole parses and get the convenience of dynamic::variable on my code 🎉🎉🎉.

@vinipsmaker2
Copy link
Contributor Author

I forgot to fill more details. it is the value returned from obj.key_begin() and obj is dynamic::variable

@breese
Copy link
Owner

breese commented Feb 10, 2018

Good catch.

The resolution gives me some headache though. There are two solutions we can use:

  1. Add a key_iterator erase(key_iterator) member function.
  2. Add an explicit conversion from key_iterator to const_iterator, e.g. key_iterator::base() -> const_iterator.

The first solution enables the user to loop over all elements and erase some of them. That is:

for (auto it = v.key_iterator(); it != v.key_end(); /* increment done below */)
{
  if (should_we_erase_element(*it))
    it = v.erase(it); // erase returns a key_iterator
  else
    ++it;
}

The second solution has the advantage of being more general, so it can be used for other functions, like insert(). However, the example above cannot be used.

I am leaning towards the second solution because:

  • We already have the looping problem with iterator erase(const_iterator) which was introduced in C++11 (via proposal N2350.)
  • There is precedence for the explicit iterator conversion with reverse_iterator::base().

@vinipsmaker
Copy link
Contributor

Why can't the example compile with solution #2?

@breese
Copy link
Owner

breese commented Feb 10, 2018

Because it has type key_iterator and erase returns type iterator.

There is no conversion from iterator to key_iterator because the latter needs an extra member variable.

@breese
Copy link
Owner

breese commented Feb 10, 2018

Regarding your comment about parsing the entire input to a dynamic::variable, you will be pleased to know that I am working on a json::parse and json::format that works on dynamic::variable without the need for Boost.Serialization.

@vinipsmaker
Copy link
Contributor

We already have the looping problem with iterator erase(const_iterator) which was introduced in C++11 (via proposal N2350.)

Okay. I've read N2350 (except for the proposed wording chapter).

I think if we apply the same solution here, the proper solution would be to add key_erase to match the begin/key_begin split. key_erase would receive a const_key_iterator and return a key_iterator.

I wouldn't mind to have a overloaded erase function instead typing the key_ thou.

Regarding your comment about parsing the entire input to a dynamic::variable, you will be pleased to know that I am working on a json::parse and json::format that works on dynamic::variable without the need for Boost.Serialization.

Surely I'll enjoy that.

@breese
Copy link
Owner

breese commented Feb 10, 2018

I have added a key_iterator::base() function to convert a key_iterator into a const_iterator in db775c4, so now v.erase(kit.base()) is doable.

Regarding the key_erase() function, I would rather have such functionality as algorithms, e.g. like the dynamic::key::count() and dynamic::key::find() algorithms, to prevent a proliferation of the variable interface.

@vinipsmaker
Copy link
Contributor

Okay. Let's leave the issue open for some time in case others want to pop in and add their comments.

I have added a key_iterator::base() function to convert a key_iterator into a const_iterator in db775c4, so now v.erase(kit.base()) is doable.

This will suit me for now. Probably I'll send a PR in the following weeks with one dynamic::key::erase. The question of a sweet spot here between convenience and a clean interface is a tricky one. Not many ready guidelines we can adhere to. But also, it's the problems we always face with novel design (otherwise they wouldn't be novel).

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

No branches or pull requests

3 participants