[boost] Re: IOStreams formal review -- extended

From: Jonathan Turkanis (technews_at_[hidden])
Date: 2004-09-14 13:41:07

"Rob Stewart" <stewart_at_[hidden]> wrote in message:

Continuing where I left off yesterday.... Again, I'll only comment on the parts
where I disagree, don't understand or believe more explanation is necessary.

> streambuf_facade::streambuf_facade
>
> You have two sections with the same heading;

I'm emulating the Dinkumware documentation style. For overloaded functions,
there is a heading like

   basic_istream::operator>>

followed by a recapitulation of the relevant part of the synopsis, followed by a
decription of each member. I've found the documentation hard to read when I
group too many overloads together, since I end up writing stuff like "this
seventh member constructs a ..."

So I split them up, but give them headers with the same name. Perhaps heading
like

   streambuf_facade::streambuf_facade (I)
   streambuf_facade::streambuf_facade (II)
   streambuf_facade::streambuf_facade (III)

or

   streambuf_facade::streambuf_facade (Default)
   streambuf_facade::streambuf_facade (Forwarding)
   streambuf_facade::streambuf_facade (From Policy)

would be better in cases like this.

> both should fall
> under the heading "Constructors."  Otherwise,
> "streambuf_facade::streambuf_facade" should be written as
> "streambuf_facade<T,Tr,Alloc,Mode>::streambuf_facade."

I don't see how adding the template arguments helps.

> ______
> The first streambuf_facade::streambuf_facade
>
> "Before the instance can be used for i/o, one of its..."
>              ^^^^^^^^
> streambuf
> ______
> The second streambuf_facade::streambuf_facade
>
> "Constructs a streambuf_facade which is ready to perform i/o,
> where the parameters have the following interpretations:"
>                                          ^^^^^^^^^^^^^^^
> meaning
>
> You don't call out function parameters like this many other
> places.  There's nothing wrong with it, but you should choose to
> explain them in a paragraph or a list and do that throughout the
> documentation.

I choose a table explaining the parameters only when I thought it would be the
more helpful format. There are cases where it seems unnecessary, e.g.,

        mapped_file_sink(const std::string& path);

    Constructs a mapped_file_sink to access the file with the given pathname.

    path - the pathname of the file to be accessed.

> ______
> streambuf_facade::open
>                  ^^
> <T,Tr,Alloc,Mode>::

Again, adding the template arguments just makes the headers harder to read, IMO.

> "Assocaites the given instance of T with this instance of
>   ^^^^^^^^^^
> Associates
>
> streambuf_facade, if there is no such instance currently
>                                   ^^^^
> T

I don't understand the ^^^^ under 'there is'. I guess I'm viewing your
corrections with an incompatible font.

> ______________
> Class template stream_facade
> ______
> Examples
>
> Where are the streambuf examples?  You said they were the
> "fundamental components" of the library.

Good point. Based on the comments of reviewers, I plan to

1. explain early in the docs the roles of stream buffers and streams in the
standard library i/o framework
2. explain that the streams in the current library are provided only as a
convenience
3. Show how to use an arbitrary streambuf with a plain istream or ostream
4. Explain that most of the examples will use streams, since these are more
familiar and will probably represent the most comon use of the library.

> _________________________________________________________________
> filtering_streams.html
> _______________________________
> Synopsis
>
> There are two, identical declarations of
> boost::io::filtering_streambuf.

Thanks, the second should be filtering_stream.

> _______________________________
> Examples
>
> Do any of these techniques apply to filtering_streambufs?

They all apply to the streambuf variants of the stream templates used.

> _________________________________________________________________
> code_conversion.html

> "* converting_streambuf: A wide-character stream buffer template
> having an interface essentially identical to
>
> Don't break the paragraph here.

Seems to be a mozilla bug.

> filtering_streambuf, with the following additional property:
> after zero or more wide-character filters have been added to the
> chain, a sequence of zero or more narrow-character filters and a
> narrow character resource may be added. A converter is inserted
> automatically between the final wide-character filter and the
> first narrow-character filter or resource."
>
> Rephrasing that will simplify it and make it clear:
>
>     ...essentially identical to filtering_streambuf, which
>     automatically inserts a converter between the last wide
>     character filter, if any, and the first narrow character
>     filter or resource.  (Any wide character filters must precede
>     the narrow character filters and resource in the chain.)

I agree that my wording is awkward, but I'm not sure I like your suggestion. How
about

    essentially identical to filtering_streambuf except that
    both wide- and narrow-character filters and resources
    can be added to it. The only restriction is that the wide-
    character components must come first. When the first
    narrow-character components is added, the libray
    automatically inserts a code_converter.

> _________________________________________________________________
> convenience.html
> _______________________________
> Overview
>
> You mention a convenience function named "copy" but the only
> function shown is operator <<.

I agree this looks funny, and will have to be changed. Refering to the
supposedly familiar overload of basic_ostream::operator<< was supposed to be a
quick way of explaining what copy does. But it clearly didn't work.

> _________________________________________________________________
> converter.html
> _______________________________
> Description
>
> "The class templates converter is...and produces a resource with
>             ^^^^^^^^^
> template
>
> wide character type"
> ^^^^
> a wide

How about:

    A resource adapter which which takes a narrow-character resource type
    and produces a wide-character resource type by introducing a
    layer of code conversion using a std::codecvt.

?

> ______________
> converter::converter
>
> Calling a constructor a member, however accurate, is odd.  Why
> not just call it a constructor?

Good point. I thought I was following the Dinkumware convention, but having just
checked, I see that I wasn't.
> ______________
> converter::open
>
> "Assocaites the given instance of Resource with this instance of
> converter, if there is no such instance currently associated;
> otherwise, throws std::ios_base::failure."
>
> "Associates" is misspelled, but rephrasing it would work well:
>
>     Makes a copy of the Resource to be used by the converter.  If
>     the converter already has a Resource, it throws
>     std::ios_base::failure.

I agree that associate/dissociate is awkward. How about attach/detach?

> I don't think "associates" leaves out an important detail,
> suggesting that the Resource is tracked by pointer and that the
> Resource's lifetime must extend to the next call to close().

I'm planning an entire section on lifetime management issues.

> "The third parameter determines the size of the buffer or buffers
> used for code conversion."
>       ^^^
> for the

My formulation sound more natural to me.

> ______________
> converter::close
>
> "Disassociates from this instance of converter any instance of
> the resource type Resource currently associated with it, calling
> cleanup functions as appropriate and destroying the associated
> instance of Resource."
>
> This is awkwardly phrased.  I suggest:
>
>     Destroys the converter's current Resource, if any, calling the
>     cleanup functions of any filters and the Resource, as
>     appropriate.

    Detaches the converter's current Resource, if any, and destroys it,
    after calling any appropriate cleanup functions

?

>
> I don't think "dissociate" (not "disassociates," BTW) is
> necessary.

Thanks. Google tells me that 'dissociate' is about twice as common as
'disassociate'. Maybe 'disassociate' should mean 'insult an attorney who hasn't
made partner.'

> _________________________________________________________________
> exceptions.html
> _______________________________
> The Boost Iostreams Library
> ______________
> Policy
>
> The text is formatted differently than on other pages for this
> heading level.

I used H5 here because I thought the higher levels looked odd. But I think it's
a moot point because I should move the rationale to the central Rationale
section.

> _______________________________
> Exception Safety
>
> "1. Resources are freed by calling destructors or
> close, if appropriate."
>
> Don't break the line between "or" and "close."

Mozilla -- up to her old tricks.

> "Conditions 2. and 3. rely on the specification of the filter and
> resource concepts. Note that there is no guarantee that a
> stream's or stream buffer's character sequences are in a
> consistent state, i.e., that data has not been corrupted, and no
> way in general to determine the state of these sequences after an
> exception."
>
> After reading this paragraph, I'm left to wonder whether removing
> and replacing the resource, as described in 3.b, means that the
> state of the character buffers is well-defined or still unknown
> as described here.  IOW, this paragraph seems to contradict 3.b
> and should be rephrased to clarify how the state of the buffers
> can be known and corrected.

Good point. After an exception, and before the chain is modified, there is no
guarantee.

> _______________________________
> Acknowledgments
>
> "Thanks to Angelika Langer and John Torjo for discussion of
> exceptions."
>
> What does "for discussion of exceptions" mean?  Did you have
> conversations with them on the subject?  Then you need to write,
> "for our discussions on exceptions and exception handling."

Yes, this sounds awkard. How about:

    Angelika Langer and John Torjo provided helpful comments on the role of
exceptions in the standard iostreams library.

?

> _________________________________________________________________
> regex_filter.html

> _______________________________
> Reference
> ______________
> basic_regex_filter::formatter
>
> "The type of object which a basic_regex_filter uses to...."
>   ^^^
> formatter is the

Do you feel the same way about elision of the subject in function documentation
which begins with "Returns"?

> "Since Boost.Function objects are implictly constructible from
> function objects with the correct signature, users of regular
> expression filters may define their own function objects with the
> correct signature and pass them to the basic_regex_filter
> constructor..."
>
> This is incomplete.  One can write free functions, lambda
> functions, etc. that satisfy Boost.Function to create formatters.

How about:

    users of regular expression filters may pass any function or function
    object to the basic_regex_filter constructor as long as its signature is
    compatible.

?

> ______________
> basic_regex_filter::basic_regex_filter

> "replace - A function which will be passed each match_results
> object in succession and whose return values will be be used as
> replacement text"
>
> Rewording this will make it clearer:
>
>     A Boost.Function object to which each regular expression match
>     is passed.  The return value is used as the filter's result in
>     lieu of the matched text.

How about:

       A Boost.Function object to which each regular expression match
       is passed.  The return value will be used in the filtered character
       sequence in lieu of the matched text.

?

> _______________________________
> Example
>
> "See Examples."
>       ^^^^^^^^

I'm not sure I see the problem. Maybe: See Examples (Regex OutputFilter).

> _________________________________________________________________
> newline_filter.html
> _______________________________
> Description
>
> "The class templates basic_newline_filter is a DualUseFilter
>             ^^^^^^^^^
> template
>
> which converts between the text file formats used by...."
>                 ^^^^^^^
> among

Why? I hope it's not "the superstition that 'between' can be used only of the
relationship between two things, and that if there are more 'among' is the right
preposition." (Fowler) :-)

> "Note: It is not known if specializations of basic_newline_filter
> other than basic_newline_filter<char> are useful. If not, the
> template parameter will be eliminated."
>
> Make this a footnote referenced from "Ch" in the Template
> Parameters section.

I wanted it to be prominent during the review. I'm planning to eliminate the
note and maybe the template parameter. Do you think it's necessary to use a
template here?

> _________________________________________________________________
> compression.html
> _______________________________
> Acknowlegments

>
> Seward for making their excellent librarys available..."
>                                    ^^^^^^^^
> libraries

at least I didn't say libaries. ;-)

> _________________________________________________________________
> file_based_resources.html
> _______________________________
> Overview
>
> "...access to memory-mapped files on Windows an POSIX."
>                                               ^^
> and
> _______________________________
> Acknowledgments
>
> "The memory-mapped file resources are based on work of Craig..."
>                                              ^^
> on the

Hmmm. Omitting the 'the' seemed more natural to me. To check common usage, I
tried the following Google query:

          [ "on work of" indebted ]

and got this: http://tinyurl.com/4q82c. So I guess you're right. ;-)

> _________________________________________________________________
> file.html

> ______
> basic_file_source::basic_file_source
>
> "Constructs...std::basic_filebuf buf opened as follows:"
>                                   ^^^
> but

I think 'buf' is residue from basic_filebuf and should be stricken.

> _________________________________________________________________
> rationale.html

> ______________
> Generic Design
>
> "2. The library can be easily extended to handle..."
>                  ^^^^^^^^^^^^^^^^^^^^^^
> can be extended easily (avoids split infinitive)

Where's the split infinitive? (Don't make me quote Fowler again ;-)

> ______________
> Chain Interface
>
> "If there is a need for it, it can easily be restored."
>                                 ^^^^^^^^^^^^^^^^^^^^^^
> can be restored easily (avoids split infinitive)

Ditto.

> _______________________________
> Planned Changes
>
> "2. The concept Buffered will be renamed MultiCharacter, since it
> is more descriptive and Buffered would be better used elsewhere."
>
> I don't understand this point.  "MultiCharacter" does not convey
> anything useful.  Even an unbuffered filter processes multiple
> characters.

Currently,

1. All filters, even those which do not model Buffered, use a buffer by default,
for efficiency.
2. Bufered filters can be used without buffering, by specifying a buffer  size
of 0.

The real difference between Buffered filters and non-Buffered filters is that
the member functions of Buffered filters accept a *buffer* of characters:

       read(Source&, char*, streamsize)
       write(Sink&, const char*, streamsize)

This is why I think MultiCharacter is more descriptive. I'd like to use Buffered
for those resources which have their own internal buffers so that the library
won't automatically add an unnecessary layer of buffering.

> "4. ...This is necessary to accommodate i/o models just as..."
>                                                     ^^^^
> such?

Yep.

> _________________________________________________________________
> adapt.html

> ______
> Return Value
>
> "Returns a Resource...delimited by first and last."
>                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> It is probably well understood by all readers, but it might be
> better to express this more formally.

Suggestions?

> _________________________________________________________________
> back_inserter.html
>
> Broken link to
>
<http://home.comcast.net/~jturkanis/iostreams/libs/io/doc/functions/classes/back
_inserter.html#synopsis>

I can't find it.

> _________________________________________________________________
> close.html
> _______________________________
> Description
>
> "This gives filters and resources an opportunity to free
> resources or reset their states in preparation for a fresh..."
>                           ^^^^^^
> state

Are you sure? "The

> _______________________________
> When is close invoked?
>
> It is premature to discuss the invocations of close() without
> showing its synopsis.  You should put the synopsis just after the
> Description section.
> ______________
> streambuf_facade and stream_facade
>
> This section is a bit confusing.  First you say that r.close()
> calls boost::io::close(r, std::ios_base::in) followed by
> boost::io::close(r, std::ios_base::out).  Then, you say that if
> "r" controls a single sequence, those calls wind up, in effect at
> least, calling r.close().

This is wrong. Call the streambuf_facade instance 'sb'. Then calling sb.close()
causes boost::io::close(r, std::ios_base::in)  and boost::io::close(r,
std::ios_base::out) to be called. These, in turn call r.close().

> ______________
> filtering_streambuf and filtering_stream
>
> You used the term "complete" here.  I recall it being defined
> elsewhere, but it would be useful if you had a single point of
> definition to which you could link whenever you mention it.
>
> The sequence of close() calls never mentions buf1.  Is there a
> mistake?  If not, some clarification is in order.

No, that's correct. buf1 only needs to be flushed using sync().

> "...each element receives two closure notifications..."
>                                ^^^^^^^^^^^^^^^^^^^^^

I agree that this should be rephrased to eliminate 'notifications', but I don't
think it's correct to say that an element receives calls.

> _________________________________________________________________
> copy.html
> _______________________________
> Description
>
> "The function template copy reads data from a given model of
> Source - or from a standard input stream or stream buffer - and
> writes it to a given model of Sink - or from a standard output
> stream or stream buffer - until the end of stream is reached."
>
> This sentence is confusing due to the number of hypenated
> phrases.  If I've got it right, it should be written like this:

As I explained yesterday, I restored the status of standard streams and stream
buffers as full-fledged models of Resource, so the hypenated phrases can be
removed.

> "The size of the temporary buffer used may be supplied..."
>                   ^^^^^^^^^^^^^^^^
> temporary buffer between the input and output streams

How about:

    The size of the temporary buffer used for data transfer may be supplied.

?

> _________________________________________________________________
> get.html
> _______________________________
> Reference
> ______________
> Description
>
> On many pages, "Description" is the topmost section.  Here, it's
> a subsection of "Reference."  Why?  This section duplicates the
> information provided in the Overview, so one of them is
> superfluous.  (This applies to many pages.)

Yes, I need to get this straight.

> ______________
> Return Type
>
> If the return type is known to be "typename
> std::char_traits<typename char_type<Source>::type>::int_type" why
> not just show that as the return type in the synopsis?

Because it's ugly :-) How about [see below].

BTW, I'm considering changing the return type to allow it to represent three
types of cases:

1. a character was extracted
2. EOF was reached
3. No characters are surrently available -- try back later.

I think this is one of the most important interface issues -- what is your
opinion?

> _________________________________________________________________
> read.html
> _______________________________
> Overview
>
> "The first overload....  The second overload...."
>
> No overloads have yet been shown, so this is premature.  Move
> these sentences to Reference|Semantics.

I see your point, but I'd like this information up front. How about 'one
overload ... the other overload'?

> ______________
> Template Parameters
>
> "T - For the first overload, a model of Source or a standard
> input stream or stream buffer type. For the second overload, a
> model of InputFilter."
>
> The fact that you must describe the parameter this way is a clear
> indication that you need to use better parameter names.  How
> about:
>
>     template<typename Resource>
>     std::streamsize read(Resource& t, ...);
>
>     template<typename InputFilter, typename Source>
>     std::streamsize read(InputFilter& t, Source& src, ...);
>
> Since the definition of "Source" refers to T's character type,
> you'll have to make reference to Resource's or InputFilter's
> character type.  With these distinctions in place, the subsequent
> Semantics sections won't need to replicate the declarations.

I think renaming the parameters is a good idea, but that recapping the synopsis
is also helpful.

> _________________________________________________________________
> reverse.html
> _______________________________
> Description
>
> "The function template reverse takes a filter and returns a new
> filter which performs the same filtering operation as the
> original filter but which is an OutputFilter if the orginal
> filter was an InputFilter and and InputFilter if the orginal
> filter was an OutputFilter. Roughly speaking, it reverses the
> direction of a given filter."
>
> The ideas here are conveyed in the wrong order

in what way?

> and why, "roughly
> speaking?"

Because I haven't formally defined the 'direction' of a filter.

>     The function template reverse reverses the filtering direction
>     of a given filter.  It turns an InputFilter into an
>     OutputFilter and an OutputFilter into an InputFilter.
>
> You mention memory overhead that can be a side effect of using
> reverse().  Are there performance issues as well?

As long as there's plenty of memory around, performance should be better than if
reverse weren't implemented in terms of one_step_filter.

> Also,
> shouldn't that paragraph be set off with "Note:" or "Warning:"?

I like "Warning".

> You mention that using reverse may not be suitable for some data
> streams, but you need to give more guidance regarding why one
> would want to use reverse().

Okay. The reason is that you may have an implementation of a filtering algorithm
only as an output filter, but want to use the same algorithm for an input
filter. As I mentioned before, some algorithms are much easier to implement one
way than the other.

> _________________________________________________________________
> seek.html

> ______________
> Function Parameters
>
> "way - ..."
>
> You refer to std::ios_base::beg twice.  The latter should be
> std::ios_base::end.  Also, "way" would be better named "dir" or
> "direction," don't you think?

The standard uses 'off', 'way', 'which'.

> _________________________________________________________________
> io_traits.html
> _______________________________
> Overview
>
> "The header <boost/io/categories.hpp> contains category tags for
> classifying models of the various filter and resource concepts."
>                                    ^^^^^^     ^^^^^^^^
>                                    Filter     Resource

Here's a good place where lowercase seems clearly appropriate. Is Filter a
Filter concept?

> "The header <boost/io/io_traits.hpp> contains the definitions of
> the metafunctions char_type and category, used to associate two
> fundamental types with each model of one the filter or resource
> concepts:"
>
> I'm not quite sure how the end of that sentence should be
> phrased, but it's confusing as is.  Perhaps this conveys the
> right information clearly:
>
>     ...used to determine the type of characters processed by, and
>     the category, or capability description, of Filters and
>     Resources.

How about:

   The header <boost/io/io_traits.hpp> contains the definitions of
   the metafunctions char_type and category, which associate with
   each model of Filter or Resource its character type and category.

?

> ______________
> Character Type
>
> This section seems out of place.  It should be titled,
> "char_type," but you haven't introduced that template yet.  I
> suggest that this descriptive text be moved to the Class Template
> char_type|Description section.
> ______________
> Category
>
> This section, too, is out of place.  I suggest moving the content
> to Class Template category|Description.

Thanks. This page was a mess, but I wan't sure what to do with it.

> _______________________________
> io_traits::type
>
> Where did this come from?  I don't understand how io_traits::type
> is related to char_type (without looking at the code, of course).

This must be a holdover from the days when io_traits was a 'blob' representing
two metafunctions. Please point out where you see this, so I can eliminate it.

> _______________________________
> Category Tags
>
> "...simply define a struct extending the existing tags."
>                             ^^^^^^^^^
> (multiply) derived from

  "simply define a struct deriving from each of the existing tags using multiple
inheritance"

?

> The table doesn't which tags are mutually exclusive.  Is that
> exclusivity enforced?  For example, a type cannot be both a
> Filter and a Resource, so only one of filter_tag and resource_tag
> may be used to create a legitimate category.  Is that enforced?

Categories are validated to a limited extent.

> Is a category tag validated in any way?  That is, what if a
> developer specifies neither filter_tag nor resource_tag.  Is the
> category tag still valid, will it produce a meaningful error, or
> will it wreak havoc?

You'll definitely get compiler errors, perhaps along the lines of
'filter_wrapper_impl<any_tag> has no member function read'. I guess I should
have a valid_category metafunction and use

    BOOST_MPL_ASSERT(valid_category<category>)

frequently.

> _________________________________________________________________
> mode.html
> _______________________________
> Description
>
> "Metafunction returning a mode tag corresponding the the mode of
> a model of one of the filter or resource concepts or of a
> standard stream or stream buffer."
>
>     mode is a metafunction that determines the mode type that
>     indicates the mode of the Filter or Resource.  The mode can
>     indicate whether the type is usable for input, output, or
>     both, and whether and how the type supports seeking.

Good -- but how about

       mode is a metafunction returning a tag structure
       indicating the mode of a Filter or Resource.  The mode
       indicates whether the Filter or Resource is usable for
       input, output, or both, and the type of seeking it supports,
       if any.

> _________________________________________________________________
> filtering_stream.html

> _______________________________
> Reference

>
> "Tr - The traits type"
>
> You should mention that std::char_traits<Ch> is the default for
> this parameter (it's in the synopsis, of course, but it bears
> repeating).

Okay

> You should also document which features of the
> traits class you need so folks that want to write their own can.
> (You might capture this information in one place to reference
> from multiple places.)

Surprisingly, perhaps, the traits type is really not needed. I'm assuming that
anyone who writes a new character type will also specialize std::char_traits (I
should say this). The traits template parameter is provided just so people can
construct streams and stream buffers with an alternate traits type, in case the
alternate traits type is required by some other API. It won't affect the
behavior of the stream or stream buffer.

For example, suppose there is an existing library that has a function of the
form

    void f(std::basic_istream<japan_char, japan_traits>&);

If I didn't provide the traits parameter, instances of filtering_stream could
never be passed to this function.

> ______________
> filtering_stream::filtering_stream (#4)
>
> The description leaves me wanting more.  IIUC, this creates a
> complete filtering_stream with no Filters that uses an iterator
> based Resource.  For input, that would mean that only the
> characters in the range [first, last) are read.  For output, that
> would mean that characters are written through *first++ until
> first == last.  If I've gotten that right, you need to say
> something like that so no one else must deduce the same
> information.  Also, this filtering_stream would be complete,
> right?

Yes.

> There's no parameter list introductory statement like, "The
> parameters have the following meaning."  In fact, you've been
> inconsistent in this on many pages.  However, adding a "Function
> Parameters" heading should be sufficient in and of itself.

> ______________
> filtering_stream::push (#1)
>
> "t - An instance of T"
>
> Can you describe this better?

Given the detailed description of T, what more is needed?

> ______________
> filtering_stream::push (#2)
>
> The signature of this function is identical to the previous push
> overload.  Thus, you should merge the two in the synopsis and
> when documenting them.

The first takes a const reference, the second a non-const reference.

> ______________
> filtering_stream::reset
>
> "Clears the underlying chain. If the chain is initially complete,
> causes"
>
> If I understand correctly, that should be worded more like this:
>
>     If the chain was complete before calling reset, it closes and
>     releases all Filters in the underlying chain.  In either case,
>     it closes and releases the Resource, if any.

The chain can't contain a Resource unless it's complete. So it should be

    Clears the underlying chain. If the chain was complete before
    calling reset,  causes each filter and  resource in the chain to be
    closed using the function close.

Thanks again.

Best Regards,
Jonathan