RE: [boost] Tuple review

From: Gary Powell (Gary.Powell_at_[hidden])
Date: 2001-06-18 11:01:17

Thanks for the review, comments follow:
  -gary-

[Ed Brey]
Design:

The nested namespace is quite convenient.  However the namespace name
"tuples" should be "tuple".  (Better reflects its usage at a given point
in a program; it's similar to giving an enum a singular name. Also
provides better correspondence to the header file name.)
[Gary]
I thought the use of tuple::tuple than tuples::tuple would be more
confusing, especially if the user did a
using namespace tuple;
In general I find it easier to read code that has namespaces that are
different names from the class names.
   
[Ed Brey]
Similarly, the "details" directory should be "detail".
[Gary]
Ok.

[Ed Brey]
A 0-tuple is not defined.  Would such be useful for generic programming?
[Gary]
Not sure. We use tuples in LL, and haven't needed it yet.

[Ed Brey]
The ref and cref are quite nice.
[Gary]
Thanks, we need them for LL.

[Ed Brey]
tie now exists in both boost and boost::tuple namespaces.  It make sense
to me to deprecate the one in utility.hpp.  Is that the plan?  The
really nice "ignore" functionality which differentiates tuple::tie adds
more weight to the issue.
[Gary]
Yes I think tuples::tie has all the current features of boost::tie.
"ignore" falls into the same category as "nil"/"null" and is a useful
concept for more than just tuples and LL. (ex. Function.)

[Ed Brey]
set_left and set_right seem likely to cause problems with right-to-left
(and top-to-bottom?) languages.  set_open and set_close would probably
be better.
[Gary]
Hmm, I'm unfamiliar with this conflict. Are there any other comments on
this?

[Ed Brey]
The tier concept could use a new name.  It took me a while to realize
you weren't (mis)using the word that meant "One of a series of rows
placed one above another."  Unfortunately, I can't think of a good
alternative.  If I can, I'll let you know; otherwise, it's this issue
isn't a big deal.
[Gary]
Caught me too, when I first looked at it. But then I don't have a good
alternative either.

[Ed Brey]
nil is undocumented.  It should probably be either documented or moved
into the detail namespace.  Is the nil/null being proposed as due to the
function library discussion compatible with this?  Assuming yes, it
would seem to keep things minimal to share the single class once it is
complete.  As far as naming goes, null is standard C/C++ terminology for
this concept of referencing nothing when one might otherwise be
referencing something, and it would be good to use the existing name for
this concept.  This is especially true if a C++ 0x might have a null
keyword that could be leveraged by simply deleting the null class.  If a
null keyword would not be drop-in replacement, then it makes sense to
call the class something besides null (e.g. nil) to avoid a future
conflict.
[Gary]
This got discussed to death on Function, with no resolution yet. Tuples can
use the Function version just fine and that is what I've proposed we do. Yet
there seems to be some hesitancy to letting function add it without another
review. If we could add it soon, I'd be glad to hold off putting tuples up
until this is resolved (I'm hoping soon, as in the next review?) But I don't
want to wait 6 months on this.

[Ed Brey]
tuple_length likewise is both undocumented and not in the detail
namespace.
[Gary]
We'll add it to the documentation.

[Ed Brey]
Documentation:

"Using the library":

It is unclear whether tuple.cpp is needed for tuples in general, or only
if tuple input and output operators are used.
[Gary]
Just the I/O streams.

[Ed Brey]
"Constructing tuples":

The depth of this section is excellent.  Unfortunately, this makes it
quite heavy for novices.  It would be useful to rearrange the documents
putting all the key points a novice needs to get started (e.g. getting
elements) towards the top.
[Gary]
Ok.

[Ed Brey]
"Note that an argument can only be left unspecified, if it has a default
initialization." is unclear.  Perhaps you mean "If no initial value for
an element is provided, it is default initialized (and hence must be
default initializable)."
[Gary]
Good point, we'll add that text.

[Ed Brey]
'tuple<X,X,X>(string("Jaba"), string("Daba"), string("Duu"))' would be
stronger example as 'tuple<X,X,X>("Jaba", "Daba", "Duu")' (assuming
that's OK).
[Gary]
Hmm.. I'm not sure what you mean by this. How so?

[Ed Brey]
"tuple<const double&>(d+3.14)    // ok, but potentially dangerous: //
the element may become a dangling reference" could be made stronger.
The element will become a dangling reference.  The only question is
whether the dangling reference will be a accessed.
[Gary]
Ok.

[Ed Brey]
"Relational perators":

Section title is missing its "O".
[Gary]
Ok.

[Ed Brey]
"Note that an attempt to compare two tuples of different lengths results
in a compile time error.": It would be worth stating here whether tuples
of different types (but same length) can be compared. (Even though the
example makes this clear, it makes reading easier not having to wait to
get down that far.)
[Gary]
Ok.

[Ed Brey]
"Streaming":

"cout << set_left('[') << set_right(']' << set_delimiter(',') << a;" is
missing a close parenthesis after ']'.
[Gary]
Ok.

[Ed Brey]
"Note that extracting tuples with std::string or C-style string elements
most likely does not work": It would be more accurate to say does not
generally work.  For applications where the string contents are
constrained, it can work just fine.  It's not a matter of probability.
[Gary]
Ok. 

[Ed Brey]
It is unclear how long the manipulators stay in affect.  I'm not sure if
this needs to be clarified or if there is a iostream standard that I'm
not remembering off hand.
[Gary]
As long as the stream exists. See the CUJ Experts articles. That's where I
learned how to do this.

[Ed Brey]
Code:

"inline" inside structure definition is superfluous.
[Gary]
Ok.

[Ed Brey]
tuple_io should not include iostream, but rather istream and ostream.
[Gary]
Ok.

Thanks again for all the comments. They are very much appreciated.

  Yours,
 -gary-

gary.powell_at_[hidden]