Re: [boost] Re: IOStreams formal review -- extended

From: Rob Stewart (stewart_at_[hidden])
Date: 2004-09-15 10:11:09

From: "Jonathan Turkanis" <technews_at_[hidden]>
> "Rob Stewart" <stewart_at_[hidden]> wrote in message:
> 
> > 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)

Those would be distinct, but not informative.

> or
> 
>    streambuf_facade::streambuf_facade (Default)
>    streambuf_facade::streambuf_facade (Forwarding)
>    streambuf_facade::streambuf_facade (From Policy)
> 
> would be better in cases like this.

Those would be better.  However, rather than rely on headings at
all, why not just let the synopsis excerpt divide things?

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

It is more correct.  I was trying to say that if you didn't use
the "Constructors" heading, but wanted to keep the
streambuf_facade headings, then they really should include the
template parameters.

> > ______
> > The second streambuf_facade::streambuf_facade
> >
> > 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.

As a reference guide, I'd expect consistency on this point so
that I could easily spot desired information.

> > ______
> > streambuf_facade::open
> 
> > "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.

As you might guess from there only being four carets, it wasn't
supposed to be under "there is."  Using a fixed width font, the
only way you can possibly understand such notation, you'll see
that I was suggesting s/such/T/.

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

So long as you meant "most" and not "all" that should do pretty
well.

> > _________________________________________________________________
> > filtering_streams.html
> >
> > _______________________________
> > Examples
> >
> > Do any of these techniques apply to filtering_streambufs?
> 
> They all apply to the streambuf variants of the stream templates used.

You apparently have more than one filtering_streams.html and I
didn't note the relative URL, so I can't find the section to
which I was referring.  However, I'm pretty certain I was trying
to say that you don't mention filtering_streambufs, only
filtering_streams in that context.

> > _________________________________________________________________
> > code_conversion.html
> 
> > 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
                       ^^^^^^^^^^
component
>     automatically inserts a code_converter.

s/component/filter/ since this only applies to filters, right?
Otherwise, it's fine.

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

Very nice.

> > ______________
> > 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 avoided "associate" altogether in my rewording.  Did I not
capture the right idea?  Anyway, "attach" and "detach" are still
not quite right because you make a copy, right?

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

That will be good.

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

Why do you need both "detach" and "destroy?"  Doesn't "destroy"
say it all?

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

LOL

> > _______________________________
> > Exception Safety
> >
> > "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.

I presume you'll mention that in your revised docs.

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

Excellent.

> > _________________________________________________________________
> > 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"?

Most text I had read in your documentation, I suspect, was
grammatically correct.  This stood out, I assume, or I wouldn't
have flagged it.  However, I don't know whether you were
consistent in omitting the article.  If so, ignore my comment.
If not, choose one convention.

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

That works.

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

That muddies the issue for me.

> > _______________________________
> > Example
> >
> > "See Examples."
> >       ^^^^^^^^
> 
> I'm not sure I see the problem. Maybe: See Examples (Regex OutputFilter).

s/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) :-)

Superstition?  No, it's just been drilled into my head.  You're
right of course, that there's nothing wrong with "between" in
that sentence.

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

I don't personally need it, but I can well imagine that folks
would need a wide character version.  Perhaps in that case, the
line endings aren't really wide character?

> > _________________________________________________________________
> > compression.html
> > _______________________________
> > Acknowlegments
> 
> >
> > Seward for making their excellent librarys available..."
> >                                    ^^^^^^^^
> > libraries
> 
> at least I didn't say libaries. ;-)

8^)

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

It should be removed.  I was probably tired and not thinking as
clearly as I should have to try correcting your English!

> > _________________________________________________________________
> > 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 ;-)

No split infinitive; probably tired again.  I do think the change
sounds better.

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

No split infinitive; probably tired again.  Either version is
fine here.

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

I understand what you're trying to say now.  I'm not fond of
MultiCharacter, but offhand, I haven't a better name.

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

[first, last)

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

There are automated tools in Boost and there will be many
readers.  It will surface again, if it is still present.

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

Each filter and resource has but one state.  (This may be a gray
area, for all I know.)

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

OK, but do give thought to clarifying the text.

> > ______________
> > filtering_streambuf and filtering_stream
> >
> > "...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.

I assume "element" refers to a filter or resource, in which case
calling a function is quite reasonable.  What have I missed?

> > _________________________________________________________________
> > copy.html
> > _______________________________
> > Description
> >
> > "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.

I still think it is useful to mention the entities sharing the
buffer.

> > _________________________________________________________________
> > get.html
> > _______________________________
> > Reference
> > ______________
> > 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].

That may be, but you're hiding the truth.

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

Given your discussions on async I/O, this is probably necessary,
but I've not spent any time considering the ramifications.

> > _________________________________________________________________
> > 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'?

I don't understand why that information needs to be in the
overview.  If one is for internal use, you don't need to
highlight it.  Without those two bullets, the overview remains
complete IMO.

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

   The function template reverse takes a filter and reverses its
   direction of data flow.  It forms an OutputFilter if the
   original filter is an InputFilter, and vice versa.

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

That's the sort of information needed in the documentation.  Of
course, I'm still unconvinced of the efficacy of the current
filtering approach, as mentioned previously.

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

Does that make them good automatically?

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

Hmmm.  I suppose you're right.  Wouldn it be better phrased as,
"...various Filters and Resources?"

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

That's fine; be sure to make "character type" and "category" be
links.

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

http://home.comcast.net/~jturkanis/iostreams/libs/io/doc/io_traits.html,
about halfway down.

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

Is it always multiple inheritance?  That's what I was trying to
convey by parenthesizing "multiply."  Otherwise, it's fine.

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

Anything to help the user get things right.

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

Fine.

> > _________________________________________________________________
> > filtering_stream.html
> 
> > _______________________________
> > Reference
> 
> >
> > "Tr - The traits type"
> >
> > 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.

<confused>
Is the traits type needed or not?  If so, what facilities must it
provide?
</confused>

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

>From this I infer that the traits type must meet the Standard's
requirements for char_traits (21.1.1).

> > ______________
> > filtering_stream::push (#1)
> >
> > "t - An instance of T"
> >
> > Can you describe this better?
> 
> Given the detailed description of T, what more is needed?

How about this:

   A Resource, if the filtering_stream is to be complete, or a
   Filters if the filtering_stream is not to be complete yet.

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

While I should have seen that, it is easy to miss and documenting
them separately is a waste of space and the reader's time.  Put
the two synopsis excerpts together and document them as one.  If
you wish, you can mention that difference between the two, but
with them adjacent, the difference should be more apparent.

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

Oops.  I did switch those around, didn't I?

Your rewrite fails to indicate what happens if the chain is not
complete.

-- 
Rob Stewart                           stewart_at_[hidden]
Software Engineer                     http://www.sig.com
Susquehanna International Group, LLP  using std::disclaimer;