incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Eric Lemings" <Eric.Lemi...@roguewave.com>
Subject RE: Tuple status
Date Wed, 02 Jul 2008 16:44:50 GMT
 

> -----Original Message-----
> From: Martin Sebor [mailto:msebor@gmail.com] On Behalf Of Martin Sebor
> Sent: Tuesday, July 01, 2008 11:08 PM
> To: dev@stdcxx.apache.org
> 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.

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

> 
>    4. assuming the answer to (3) is yes, 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.

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

    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.

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

> 
>    7. the right place to #define _RWSTD_XXX macros is <rw/_defs.h>

Which ones in particular?

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

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

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

Mime
View raw message