Bug 1294

+1 for the new Read method, however I don't really see the point in PeekU8.

I don't question its usefulness, it might be used indeed. However it's easy to 
do a "PeekU8" with a ReadU8 and a subtraction on the iterator. Sure using a 
Peek is more efficient, but... do we need such an optimization ?

Hence, please explain with an example the need for a Peek function.

Here is the piece of code where I'm using Peek:

--- cut ---
  // parse the rest of nested blocks
  while (!start.IsEnd () && start.PeekU8 ()!=CCN_CLOSE)
    {
      Ptr<Block> block = Block::ParseBlock (start);
      m_nestedTags.push_back (block);
    }
  
  if (start.IsEnd ()) //should not be the end
      throw CcnbDecodingException ();

  start.ReadU8 (); // read CCN_CLOSE
--- end cut ---

I totally agree that Prev () call will give exactly the same functionality (I kind of missed it existence when I was coding), but presence of PeekU8 () unifies Buffer.Iterator with standard streams (e.g., std::istream), which may not have concept of putting things back.

Good point. I could however, add that you could need also a Peek16U and so 
on... 

Anyway, I think that Peek8U is probably the most used one, so we might as well 
add the others when (and if) needed.

Another point is about code duplication: the PeekU8 implementation imply 
duplicating code (some is shared between Read and Peek).

So either you implement Peek through Read or the opposite. Core duplication 
means a 99% bug probability when you'll have to update the code of one of the 
two similar functions.

Cheers,

T.

I agree with tommaso about the implementation of PeekU8.

I do not ming about ::Read but ::Read(Iterator,uint32_t) should be implemented in terms of ::Write(Iterator, Iterator) (it will be oh so much more efficient). Also, if you do this, it would be nice and be coherent and provide all combinations: ::Write(Iterator, uint32_t), ::Read(Iterator,Iterator)

Created attachment 1382 [details]
Patch

I changed implementation of Read and PeekU8.  Now there is no code duplication for PeekU8 and Read(Iterator,uint32_t) is implemented using Write(Iterator,Iterator) and Next(uint32_t).

(In reply to comment #5)
> Created attachment 1382 [details]
> Patch
> 
> I changed implementation of Read and PeekU8.  Now there is no code duplication
> for PeekU8 and Read(Iterator,uint32_t) is implemented using
> Write(Iterator,Iterator) and Next(uint32_t).

It should be the other way around. i.e., ReadU8 should be implemented in terms of PeekU8

Created attachment 1785 [details]
new patch

This inverts the PeekU8 and ReadU8 dependency.

Is it ok to push ? The new functionality might be useful and it doesn't hurt.

I did not review the whole patchset again but +1 for merging

pushed in changeset:   10624:8711242c4fe6