NFD » ndn-cxx

Does this feature one need to add 1 functions or are we going to implement the same overloads of std::vector::insert??

Answer to note-2: For the purpose of #2879, only one function is needed.

Should the function do any kind of checking or just call the insert method in the vector?

Reply to note-4: I do not have an answer. The assignee should figure this out.

Added UT

I just read again the specification and the function that should be added should not use const's.

Can I change the prototype of the function to

element_const_iterator
insert(element_const_iterator pos, const Block& element);

Answer to note-9:

You can, but gcc46 on Ubuntu 12.04 doesn't like const_iterator in std::vector<..>::insert, so there would be lots of trouble to make it work.

I just saw the error in Jenkins. I developed it with the const iterators because looking at the interface there is no function that returns a non constant reference to the elements vector.

With the consts this would work

  Block::element_const_iterator it = masterBlock.find(tlv::NameComponent);
  it = masterBlock.insert(it, firstBlock);

Without the const's I am not sure if we have any scenario where we can use the function

Can you give me an example of usage of the insert without the const?

I agree with the suggestion of std::advance from note-12

The second solution from note-12 makes the most sense to me.

Similarly, Block::erase() requires a non-const iterator. Should it be changed to use a const iterator or be overloaded to allow a const iterator to be provided?

I tried a similar solution to note-12's second solution to get an element_iterator for use with Block::erase(). I wasn't able to get std::advance to compile, but calling boost::next with the same arguments and setting a new element_iterator to it's return value did seem to work. The linked reference recommended using either of the functions. I wasn't able to test it, but it did compile.

I just implemented the insert with the const iterators as advised and launched a compilation in travis:

https://travis-ci.org/joaopapereira/ndn-cxx/builds/70247481

Looks good.

I am going to implement the erase now.

Shall we keep the current versions of erase?

PS: The jenkins script is all messed up it compiles the code 4 times :s

Answer to note-18:

No, don't keep old erase.

If ndn-cxx-breaks detects regression, I'll reconsider.

It's intentional for Jenkins to build ndn-cxx- multiple times.
Each build is using a different ./waf configure line.

The latest review in gerrit says that the functions first should be moved to the body and only then this Issue should be resolved.

Well not sure if I agree with it. As I said before I do not mind to create a new task under #1694 to solve the problem in the Block class, but it will involve creating UT and UT with the current layout of the class are not worth it for Block::erase.

But if you believe that the order should be first move to cpp and then solve this Issue, then open the issue and tell me what it is and I will solve it.

PS: I see the issue 1694 as already a patch but it is 1 year old, maybe it will not be usefull now and only changes things in name and name-component, not "block" maybe we should create a separated issue for block

Reply to note-21:

The order is:

1. Move function definitions from block.hpp to block.cpp. This commit refs #1694, not a separate issue.
2. Add insert and modify erase including unit testing. This commit refs #2998.

So will we have 2 commits for #1694? Or the commit must move the functions from name and name-component?

So will we have 2 commits for #1694? Or the commit must move the functions from name and name-component?

There is no one-to-one relation between Redmine issue and commit.

Forget about Name and name::Component.
Just upload a commit for Block with refs #1694.

Gerrit for the change in Block http://gerrit.named-data.net/2223

What shall be the name of the define?

HAVE_VECTOR_CONST_ITERATOR
HAVE_STD_VECTOR_CONST_ITERATOR
HAVE_VECTOR_CONST_ITERATOR_INSERT_ERASE

I prefer NDN_CXX_HAVE_VECTOR_INSERT_ERASE_CONST_ITERATOR.

@Davide?

Junxiao Shi wrote:

I prefer NDN_CXX_HAVE_VECTOR_INSERT_ERASE_CONST_ITERATOR.

@Davide?

Sounds good to me.