RE: [boost] Re: Reminder: Tokenizer formal review

From: Gary Powell (Gary.Powell_at_[hidden])
Date: 2001-06-01 09:59:27

Hi John,

>>
> Also, shouldn't the second argument InputIterator end, be 
InputIterator
> const &end ?
> (I just looked over some STLPort code for things like std::copy, 
and it
> makes a copy of "end" so no doubt there are conversion comparison 
issues
> that I'm forgetting, so this is a pretty weak suggestion.)
> 
> Same for the the constructor in the class offset_separator?
> 

The reason begin is a reference is that it needs to be changed by the 
parsing functor. Because the functor does not need to change end, I 
just followed the convention of the STL and used pass by value.
<<
The suggestion was to use a const &. The reasoning was that for a large
object which isn't modified it's generally better to not copy it. Of course
for your vector<int>, the iterators are often just pointers, so the copy
issue is pretty much moot.

>>
> For template class punct_space_seperator, why not use a second class
> element, the charTraits = std::char_traits<Char> > to mimic the std 
use of
> basic_string. (Pass in the extra template argument to basic_string<>
> 
> 

Could you explain this one more and maybe give an example? I am not 
sure I fully understand.
<<
The definition of basic_string is
template<class CharType, class CharTraits = std::char_traits<CharType> >,
in your class punct_space_seperator, you allow the basic_string's to vary by
only one of the  two parameters. I was suggesting for completeness that you
just add the other default template class type. (CharType).
At this moment I'm not sure where I would use it, other than to define my
own special punctuation which may or may not need a specialized character
traits. Hence the request.

>>
> For the class offset_seperator, why not use an vector::iterator in 
place of
> unsigned int curoffset_ ? Then instead of testing offset_.size(), 
you could
> test curoffset_ != offset.end(),
> and instead of doing c = offset_[curoffset_]; you could do c = 
*curoffset_;
> (A small efficency I suppose, but I have a tendency to use 
iterators rather
> than index's as if for some other reason the container changes, 
often the
> rest of the code doesn't have to.)
> 

The big reason for using a index instead of an iterator is operator= 
and copy constructor. A copy of the iterator will not be valid for 
the copy of the vector. However, the copy of the index, will be valid 
for the vector copy. Thus, the compiler generated versions of these 
are correct.
<<
Good point, and note to that effect in the code might be useful. (Again
looks like a case of premature optimization on my part.) 

>>

Thanks for your comments,

<<
You're welcome. I hope to actually run and play with it in the next few
days, but it looks good so far.

  -gary-

gary.powell_at_[hidden]