Boost logo

Boost :

From: jbandela_at_[hidden]
Date: 2001-06-03 19:43:10


Responses are below comments

> 2. My compiler couldn't match the container & tokenizer-functor
constructor
> of "tokenizer" with the first three tokenizer tests. If I
commented out the
> "const" from the declarations of "test_string," that code passed.
I guess
> the qualification screws up the type matching.

I think that is due to a mistake on my part. The constructors that
take Container, should take a const reference rather than just the
reference they do now. Thanks for pointing it out.
 
> 3. Even though all instances of "offset_separator" have constructor
> arguments, the constructors of "tokenizer" and "token_iterator"
insist that
> the _possibility_ of default constructing the tokenizer-functor
type must
> exist. Since "offset_separator" has no default constructor, my
compiler
> chokes on it. (The errors are highlighted in "tokenizer.hpp".)
>
offset_separator does not have a default constructor, because I am
not sure of what the semantics of a default constructed
offset_separator should be.

> * If detail::tokenizer_base is expected to be the base class for
a "Base"
> class that is a template parameter for templated member functions of
> tokenizer_policy, then tokenizer_base should move to the
outer "boost"
> namespace, because it is a published component. If not, then don't
publish
> it in the documentation and change the other items in the file to
not
> require it. The token_iterator_generator and make_token_iterator
templates
> directly depend on the existence of tokenizer_base. The
tokenizer_policy
> class template practically depends on tokenizer_base, because the
class
> template uses the exact syntax of tokenizer_base. (Run-time
information
> from one class to another should be a member function, to allow the
> possibility of on-the-fly generation of the information.)
>

tokenizer_base is unpublished because the user should never have to
use it. Based on other comments, tokenizer_policy should also be
unpublished.

> * A generator, in STL lingo, is a function object that takes no
arguments
> and returns a value, so "token_iterator_generator" has an
inappropriate
> name. Also, a traits class that just provides a type should only
have an
> interface that consists just of that type (we'll call it "type").
You mix
> metaphors by having a main "type" and other published types. That
looks
> confusing. Either rename "type" to something else or unpublish the
helper
> types.

The name generator is used because iterator_adapter uses it. The
other typedefs should be private.

> * I don't like the interface that involves the first iterator
argument
> being a reference. Other libraries and STL almost always take
iterator
> arguments as values. The std::codecvt facets use your idea a
little bit.
> An alternative would be to return a std::pair<Iterator, bool>,
where the
> second part is what you already return and the first part is the
new next
> iterator.

I do not see why the reference is so bad, since these functions will
only be called from tokenizer_policy and not by the user.
 
> * The csv_separator and punct_space_separator could take another
template
> argument, to define a traits class for comparison, like strings and
I/O do.
> The default value would be std::char_traits<Char>.

Good idea.

> * For csv_separator, shouldn't the separating character go first
in the
> constructor? I think that would be what changes the most.
>

I personally have changed the escape character more. For example, I
have changed it before to ^ since in files that include pathnames,
since it is a pain to escape out each \ in the pathname in Windows.

Thanks for your comments,

John R. Bandela


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk