Re: Reminder: Tokenizer formal review

From: jbandela_at_[hidden]
Date: 2001-05-31 18:45:34

Gary,
My responses are below the questions/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.)
> 

I believe that tokenizer_policy::dereference and increment have
assert(b.valid_).  I believe if you include cassert, assert is in 
namespace std. If someone knows differently about assert, I would 
appreciate being enlightened.

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

Good point. I originally wrote the function totally inline. Thus 
there were iterator increments all over the place. When I factored 
out the escape sequence processing. I left the structure the same. I 
tend to agree with out that for(;iter!=end;++iter) is the preferred 
form when possible.

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

> same class, member function operator(), wouldn't it be easier to 
read,
> 
>   if (!breturnpartiallast_ && (i < (c-1) ) ) return false;
> 
> than the nested if's.
> 

Looks good.

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

I think that will work and be more efficient.

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

Good point. Thanks.

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

They should be private.

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

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

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

Thanks for your comments,

John R. Bandela