Mailing-List: contact dev-help@stdcxx.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@stdcxx.apache.org
Received-SPF: pass (athena.apache.org: domain of msebor@gmail.com designates
 64.233.178.240 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:date:from:organization:user-agent:mime-version:to
         :subject:references:in-reply-to:content-type
         :content-transfer-encoding:sender;
        b=medufTCgi4m14foPeDr+4trt/u7GoDBjN2qtZZOoSV12+Xf+iZkMJhIBFuS5MlqPMP
         oc6lPf2mZ5Pl9X5BVufguswGVg0eYPqT+C3f+MHmS5e9/kd0NCRURXfPB0rRBL4gpHiT
         0oqwDrYxEpSnuPxBTHkwzPe4oYHVmgvSLWUDo=
Message-ID: <486CED2A.9040400@roguewave.com>
Date: Thu, 03 Jul 2008 09:15:54 -0600
From: Martin Sebor <sebor@roguewave.com>
Organization: Rogue Wave Software, Inc.
User-Agent: Thunderbird 2.0.0.14 (X11/20080501)
MIME-Version: 1.0
To: dev@stdcxx.apache.org
Subject: Re: Tuple status
References: 
 <CFFDD219128FD94FB4F92B99F52D0A4901DB0607@exchmail01.Blue.Roguewave.Com>
 <CFFDD219128FD94FB4F92B99F52D0A4901DB060A@exchmail01.Blue.Roguewave.Com>
 <CFFDD219128FD94FB4F92B99F52D0A4901DB060F@exchmail01.Blue.Roguewave.Com>
 <486B0D2D.4090500@roguewave.com>
 <CFFDD219128FD94FB4F92B99F52D0A4901DB0612@exchmail01.Blue.Roguewave.Com>
In-Reply-To: 
 <CFFDD219128FD94FB4F92B99F52D0A4901DB0612@exchmail01.Blue.Roguewave.Com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Sender: Martin Sebor <msebor@gmail.com>

Eric Lemings wrote:
>  
> 
>> -----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.

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.