Re: [boost] Hello?

From: David Abrahams (dave_at_[hidden])
Date: 2005-09-15 21:41:48

"Robert Ramey" <ramey_at_[hidden]> writes:

> David Abrahams wrote:
>
>> At this point, if you make any change I think you should add a note
>> for those of us who are too clever by half, saying that, yes, it's
>> really okay to overload in boost::serialization, and describing the
>> trick used.
>
> no problem with that.
>
> I've recently checked in changes in the documentation in accordance with 
> this discussion.  I believe that they will be satisfactory.
>
> BUT there is a problem.
>
> I recast the section Archive Concept / Saving in the table format  used by 
> SGI.  I'm very dissatisfied with this for esthetic reasons.  I may be more 
> sensitive to this than other people because I use a monitor than can display 
> in portrait mode and that is the way I always use it.  (Its in explicable to 
> me that everyone doesn't do this!).

In principle I agree.  However, I like having two portrait-sized
images side-by-side :)

I think the aesthetics of the presentation are mostly OK; your biggest
problem is with the long code in the table cells.  For the MPL book we
dealt with this by inserting line breaks in the cell, e.g.:

    ar.template 
         register_type<
           T
         >()

That's an extreme example -- I probably would only use one newline in
that one.

Oh, part of the problem is that the "Expression" column is full of
things that aren't valid expressions, and are therefore much longer
than they should be!

Okay, it looks like I'm going to have to go through the whole thing
and add my comments :(

> An Archive object is a sequence of bytes created from an arbitrary
> nested set of C++ data structures. Here we describe the Archive
> concept. This is a list of requirements that a class must fullfill in
> order for a class to be used to save and load and arbitary nest set of
> C++ structures. Strictly speaking, there are two Archive concepts. One
> for saving data and one for loading it back. In this document we use
> the term Archive concepts for either one. Which one we mean should be
> clear from the context.

That's not really acceptable.  The proper thing to do is define the
Archive concept of which LoadingArchive and SavingArchive are
refinements (choose better names; I picked those to be consistent 

> Saving
> 
> Associated types
>
> +-----------------+-------------------+-------------------------+
> |boost::mpl::bool_|Archive::is_saving |boost::mpl::true if this |
                              missing "_ "---------------^
                                        should be "Archive"--^^^^

                                         rewrite:

                                         boost::mpl::true_ if data can
                                         be saved to an object of type
                                         Archive.

> |                 |                   |is an Archive to which   |
> |                 |                   |data can be              |
> |                 |                   |saved. boost::mpl::false |
> |                 |                   |otherwise. For an        |
> |                 |                   |example showing how this |
> |                 |                   |can beused, see the      |
> |                 |                   |implementation of        |
> |                 |                   |split_free.hpp.          |

The last sentence should be in a footnote or in the main body of the
text below.

> +-----------------+-------------------+-------------------------+
> |boost::mpl::bool_|Archive::is_loading|boost::mpl::true if this |
> |                 |                   |is an Archive from which |
> |                 |                   |data can be              |
> |                 |                   |loaded. boost::mpl::false|
> |                 |                   |otherwise.               |
> +-----------------+-------------------+-------------------------+

All table columns need column headers.

The first column doesn't make any sense.  It's the name of a
template.  Strike it.

It seems like the second column is a Valid Expression just like any
other.

[is_saving/is_loading are poorly chosen names for compile-time
constants: they strongly imply state.  I would use can_save and
can_load]

> Notation
> 
> Archive   A type that is a model of the Archive concept.
> T     A type that is a model of the Serializable Concept.
> ar    Object of type Archive
> t     Object of type T

You should use something like x instead of t.  Having T and t in the
same context is slightly confusing and it makes it very difficult to
verbally discuss what's in the table.

> Valid expressions
> The following expressions must be valid. \

Kill the backslash-------------------------^

> +----------+---------------------+---------------------------------+
> |Name      |Expression           |Return type                      |
> |          |                     |                                 |

Note: the "Name" column is a luxury.  If you find it helps the
aesthetics it's okay to leave it out.  I suggest you also drop the
word "return" from "return type"

> +==========+=====================+=================================+
> |Save      |ar.save_binary(const |void                             |
> |Binary    |void *, std::size_t) |                                 |

Okay, that's not a valid expression.  You should write something like:

      ar.save_binary( p, n )

and then add a Notation section that includes something like

    n    a non-negative value of integral type
    p    a pointer to n bytes of storage containing POD data
      
> +----------+---------------------+---------------------------------+
> |Save      |ar << t              |Archive &                        |
> |          |                     |                                 |
> +----------+---------------------+---------------------------------+
> |Save      |ar & t               |Archive &                        |
> |          |                     |                                 |
> +----------+---------------------+---------------------------------+
> |Register  |ar.template          |void                             |
> |type      |register_type<T>()   |                                 |
> +----------+---------------------+---------------------------------+
> |Library   |ar.library_version() |unsigned int                     |
> |Version   |                     |                                 |
> +----------+---------------------+---------------------------------+
> 
> Expression Semantics
> +----------+---------------------+---------------------------------+
> |Name      |Expression           |Semantics                        |
> +----------+---------------------+---------------------------------+
> |Save      |ar.save_binary(const |Appends count sequence of bytes  |
> |Binary    |void * address,      |starting at address to the       |
> |          |std::size_t count)   |archive.                         |

Again, not a valid expression.  Apply the same correction used earlier
here.  I don't see why you're not using a single table with 4 columns
including Semantics and Return type.  Cross-referencing two tables is
more work for the user than it should be.

> +----------+---------------------+---------------------------------+
> |Save      |ar << t              |Appends a sequence of bytes      |
> |          |                     |corresponding to the value of    |
> |          |                     |type T to the archive.           |
> +----------+---------------------+---------------------------------+
> |Save      |ar & t               |Appends a sequence of bytes      |
> |          |                     |corresponding to the value of    |
> |          |                     |type T to the archive.           |

That description of the semantics doesn't seem detailed enough to me.
Doesn't it need to call 

  serialize(ar, t, some-version)

at some point?  I bet it's not enough to append any arbitrary sequence
of bytes that I determine corresponds to the value of type T.

Remember, this table has to tell me what I need to do
in my archive to make it work with the library.  Concept tables are
not primarily for users of the concept's models (who should have
specific documentation for each model), they're for implementors of
the models.  

Don't use the container requirements tables as examples, because the
container concepts are almost useless for generic programming.  Look
at the iterator requirements.

> +----------+---------------------+---------------------------------+
> |Register  |ar.register_type<T>(T|Appends a sequential integer to  |
> |Type      |* t = NULL)          |the archive. This integer becomes|

What is a "sequential integer?"

The expository writing there really should go in a footnote or in the
main body of the text below.  These tables should be both minimal and
complete.

> |          |                     |the "key" used to look up the    |
> |          |                     |class type when the archive is   |
> |          |                     |later loaded. This process is    |
> |          |                     |referred to as "class            |
> |          |                     |registration". It is only        |
> |          |                     |necessary to invoke this function|
> |          |                     |for objects of derived classes   |
> |          |                     |which are serialized through a   |
> |          |                     |base class pointer. This is      |
> |          |                     |explained in detail in Special   |
> |          |                     |Considerations - Derived         |
> |          |                     |Pointers.                        |

Again, not a valid expression.  Maybe you mean

       ar.register_type((T*)0)

??

The semantics column once again seems far too much geared to users of
models of the concept and gives little information to the potential
archive implementor.  

> +----------+---------------------+---------------------------------+
> |Library   |ar.library_version() |Returns the version number of the|
> |Version   |                     |serialization library that       |
> |          |                     |created the archive. This number |

So, I'm implementing an archive.  How am I supposed to acquire the
correct version number?

Take the following expository stuff out of the table.

> |          |                     |will be incremented each time the|
> |          |                     |library is altered in such a way |
> |          |                     |that serialization could be      |
> |          |                     |altered for some type. For       |
> |          |                     |example, suppose the type used   |
> |          |                     |for a count of collection members|
> |          |                     |is changed. The code that loads  |
> |          |                     |collections might be conditioned |
> |          |                     |on the library version to make   |
> |          |                     |sure that libraries created by   |
> |          |                     |previous versions of the library |
> |          |                     |can still be read.               |
> +----------+---------------------+---------------------------------+

> Loading
>
> A type that models the Archive concept is a class with the following
> members:
> 
> The template parameter T must correspond to a type which models the

  "correspond to" or "be?"

AFAICT, you mean:

  T must be a model of Serializable.

> Serializable concept.
> 
> template<class T>
> iarchive & operator>>(T & t);

What is this?  operator>> is a binary function.  Perhaps you mean this
to be a member of something, but if so, it's unclear of what.

What is iarchive?  Is this documentation of the Archive concept or of
some specific model?  If the former, is operator>> really required to
return iarchive&?  If the latter, it doesn't belong on a page titled
"Archive Concept."

I just snipped out the rest of the documentation because all of the
questions I am asking in this break between quotations apply to most
of it.  I can't tell what you're trying to do.  It seems like you
don't quite have a clear understanding of what a "Concept" is yet.

> I know I didn't choose the orginal form without thinking about it.
> Looking back I realize now I was strongly influenced by the
> presentation in "STL Tutorial and Reference Guide" which lays out
> the requirements in a form similar to the one I chose.

I haven't seen it, but it would shock me if Dave Musser did it wrong.

> Aside from any errors that remain which I will be happy to fix in
> any case, I would like to leave the requirements in the dictionary
> text form as it was originally.  

?? Dictionary text form??

> If I move to the table form for the Archive Concept, then in the
> interest of consistency I would also have to do that with the
> section Serializable Concept which would be a much larger task.  So
> what I would like to do is revert to the dictionary format for the
> saving archive concept.  I've just uploaded updated document files.
> It contains one format for the save archive concept and the other
> for the load archive concept.  Interested parties are invited to
> comment before I make a final change in one or the other.

Unless I'm missing something huge (happened once this week already),
the format is the least of your problems.  The content is wrong; it
doesn't do what a concept description is supposed to do.

-- 
Dave Abrahams
Boost Consulting
www.boost-consulting.com