Eric Lemings wrote:
-----Original Message-----
From: Martin Sebor [mailto:[EMAIL PROTECTED] On Behalf Of Martin Sebor
Sent: Tuesday, July 01, 2008 11:08 PM
To: [email protected]
Subject: Re: Tuple status
Eric Lemings wrote:
I got this problem fixed with my latest change.
The tuple test suite needs to be bolstered (suggestions for
additional
test strategies greatly appreciated BTW) but the implementation as a
whole is stable enough for code review.
Excellent! It looks very clean -- I like it. Just a few quick
questions and comments on the implementation (I haven't looked
at the tests yet):
1. <tuple> should only #include the traits header(s) for the
the traits it needs, not all of <type_traits>, and use only
the __rw_xxx traits (not std::xxx)
I stopped including individual headers at one point when I reached
7 or 8 headers so I just included the whole lot. That was before
my last big change however so I could probably go back to individual
headers.
One of the design goals of traits is to make it possible to NOT
#include <type_traits> but rather our own implementation of the
same (for namespace cleanliness, among other things). Travis is
working on a scheme that should make #including the type traits
headers less tedious. Your feedback based on your experience in
<tuple> should help us come up with a convenient scheme.
2. would it be possible (and would it make sense) for <tuple>
to forward declare reference_wrapper instead of #including the
whole definition of the class to reduce the size of translation
units that don't use the class?
I believe it would.
3. why is is the base class __rw_tuple rather than std::tuple?
(seems there's a lot of duplicate code between the two)
It's primarily because of the specialization for pairs. For that
specialization to work with the tuple helpers, it needs to have the
same structural layout (i.e. recursive inheritance and data storage)
as the generalization. The best way for both to work in the same
helpers (mini-algorithms really) is to use a common base.
I agree though: there is a lot of duplication and if it were not for
the special pair ctors and assignment operators, there would probably
be only one class template.
Hmm. That's too bad. The prevailing opinion among the C++ committee
is that pair is exceeding less useful now that we have tuple and that
it would be best removed or deprecated or some such. It would be a
shame to compromise the design of type just to accommodate something
that most of us seem to think is a wart and shouldn't be used. If
there's just no way to have a clean tuple because of pair we might
want to formally propose to eliminate the coupling between the two.
We have at most 2 months to do this.
4. assuming the answer to (3) is yes,
^^^
I meant NO here.
are the casts in tuple
copy ctor and copy assignment necessary or might there be
a cleaner way to accomplish the same thing? if not, please
consider adding a brief comment explaining why they are
important
The casts are necessary to make sure the public ctors and operators
bind to the correct ctors and operators in the internal base class.
I was afraid of that.
The copy ctor for example:
tuple (const tuple& __tuple)
: _Base (_RWSTD_STATIC_CAST (const _Base&, __tuple)) { /* empty
*/ }
The object type and argument type are both `std::tuple<_TypesT...>'.
We want this ctor to call the corresponding ctor in the base class.
Namely,
__rw_tuple (const __rw_tuple& __tuple)
...
Without the cast, the argument type passed to the internal ctor is
still `std::tuple<_TypesT...>' and, because of the template ctors,
the compiler would therefore bind to the templated copy ctor wisely
deeming this ctor a better "fit".
Would this be prevented by not deriving tuple from __rw_tuple (or
not deriving it directly)? FWIW, it seems to me that in order to
implement the space optimization we discussed this morning, we
might need to make some changes in this area anyway. We should
keep the cast issue in mind while designing a solution.
template <class _HeadU, class... _TailU>
__rw_tuple (const __rw_tuple<_HeadU, _TailU...>& __tuple)
...
This is not what we want. (On a side note, it appears that the C++0x
mode in GCC 4.3.x -- because of rvalue references I'm guessing -- is
much more type strict than current and past versions.)
In the helper functions, the accessors are defined in the internal
class template. (The public class template doesn't even really have
a head and tail.) So in this case, the casts are needed to expose
these internal accessors.
5. assuming the answer to (3) is yes, would it be better to
define the [in]equality comparison for __rw_tuple as (possibly
member) functions called _C_equal and _C_less to avoid the
casts in the same operators in tuple and reduce the size of
the overload set?
I tried doing it that way but the terminating specialization proved a
little tricky (they would have to be added to the empty specialization)
so I just wrote them outside the template.
I can appreciate that. The main reason to avoid overloading
the operators is to reduce the size of the overload set. Each
time an operator is referenced, all of its overloads are thrown
together to form a set of candidates from which the compiler
chooses the best match. The smaller the set, the less of chance
of ambiguities and (presumably) the faster the compilation. (No,
I don't have any numbers showing how much faster.)
6. we can and should remove the _RWSTD_NO_EXT_CXX_0X guards
from all implementation headers under include/rw/
The assumption is that only we include those headers and presumably
we only include them in the right public headers and IF end users
include these internal headers (which they shouldn't do) then they
should get errors?
End users are expected to go through the public headers, and they
should get "friendly" errors when they use them in the wrong mode
(i.e., C++ 0x headers in C++ 2003 mode). We use the internal
headers and we don't need no stinkin' friendly errors -- we know
what we're doing ;-)
7. the right place to #define _RWSTD_XXX macros is <rw/_defs.h>
Which ones in particular?
_RWSTD_FORWARD() and _RWSTD_MOVE().
Incidentally, you might be pleased to know that there's no need
to uglify macro parameters: they can't conflict with user-defined
macros. So it's fine to define, for example, _RWSTD_FORWARD like
so:
#define _RWSTD_FORWARD(T) ...
instead of
#define _RWSTD_FORWARD(_TypeT) ...
8. why is std::ignore typedef'd to __rw_ignore when the latter
isn't used anywhere (AFAICS), and shouldn't it be defined in
a .cpp file to avoid multiply defined symbols?
It can be used in the tie() function which I still need to implement.
9. shouldn't tuple_element take size_t as template argument
as per LWG issue 755
Yes.
10. the non-trivial bits could use some comments :)
Non-trvial? Which parts are non-trivial?
Hehe.
Just point them out and I'd be glad to comment them.
The casts, for one. Some of the forwarding seems less than obvious
to me, too. The partial specialization and why it's necessary. You
know, comments that help the rest of us understand the
implementation :)
In __rw_tuple, are the accessors necessary? Would it be possible
(and cleaner) to declare the appropriate specialization(s) of
__rw_tuple a friend instead and have it (them) access the data
members directly?
I try to avoid friends as a general rule but I did consider it and
IIRC the conclusion I came to was that it would really make a mess.
A few notes on style :)
a) please re-read points (0), (1), and (5) here:
http://www.nabble.com/forum/ViewPost.jtp?post=18174685
Yeah, need to update for 1 and 5. Don't see 0 though.
Sorry, by (0) I meant the first point about _RWSTD_NO_EXT_CXX_0X.
Martin
b) please use the name _TypeT (as opposed to _Type) for template
parameters
What's wrong with _Type? :)
Okaaayy fine. (Still doesn't make sense though and I still think this
convention ought to be relaxed.)
c) in ctor initializer lists spanning multiple lines, please
avoid dropping the colon or the comma; i.e.,
struct A: Base {
int _C_i;
A (int i): Base (), _C_i (i) { /* empty */ }
or (for long initializer lists):
A (int i):
Base (very_long_sequence_of_arguments),
_C_i (i) {
// empty
}
d) I realize we haven't formally closed the VOTE on the variadic
template parameters but I think we're all in agreement so we
might as well start using the right naming convention (_TypeT
and _TypesT, etc.)
Not a problem... for me at least. I think it will make this code in
particular though harder to read. The reader will constantly have to
remind himself/herself "_TypeT is the head, _TypesT is the tail".
Brad.
