RE: [boost] Reminder: Tokenizer formal review

From: Gary Powell (Gary.Powell_at_[hidden])
Date: 2001-05-31 18:01:13

I'm still digging through this code, but I have a couple of
question/comments.

file: tokenizer_policy.hpp
fns : boost::tokenizer_policy::dereference and increment, both have a
statement
"using namespace std;"
but I don't see any calls to functions from that namespace. What am I
missing here? Or is this leftover junk? (I did see the comment that "assert"
was sometimes, global, but I thought it was always a MACRO. hence no
namespace issues are possible.)

same comment for
file: token_functions.hpp
fn boost::csv_seperator::template<..> operator()
(no call to assert here at all.)

In (again)
fn boost::csv_seperator::template<..> operator()
I would prefer the loop while (next != end) { ..... ++next; }
be replaced with
          for(;next != end; ++next) {}

it's not a big deal, but with loops that have an increment like this, IMO it
reads easier, as in I'm sure that "next" is going to be incremented. Took me
a minute to verify that it was indeed the case.

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?

same class, member function operator(), wouldn't it be easier to read,

  if (!breturnpartiallast_ && (i < (c-1) ) ) return false;

than the nested if's.

and 
   for (; i < c && next != end; ++i, ++next) 
       tok += *next;
to avoid the extra copy that 
    next++ makes?

In general the constructors with all default arguments should be declared
"explicit" for the  well known reasons of inadvertent conversion.

Why are the data members of cvs_seperator public? or did you mean to use a
"class" instead of a "struct".

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<>

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.)

I'm going to spend some more time with this library as it looks really
useful. And I know that all the comments are really nitpicky. Sorry, but
that is the way I am.
 -gary-