Boost logo

Boost :

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

My responses are below the questions/comments.

> file: tokenizer_policy.hpp
> fns : boost::tokenizer_policy::dereference and increment, both have
> statement
> "using namespace std;"
> but I don't see any calls to functions from that namespace. What am
> 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
> 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
> 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
> 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
> "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 =
> (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
> 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

Boost list run by bdawes at, gregod at, cpdaniel at, john at