stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: Tuple status
Date Wed, 02 Jul 2008 05:07:57 GMT
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)

   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?

   3. why is is the base class __rw_tuple rather than std::tuple?
   (seems there's a lot of duplicate code between the two)

   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

   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?

   6. we can and should remove the _RWSTD_NO_EXT_CXX_0X guards
   from all implementation headers under include/rw/

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

   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?

   9. shouldn't tuple_element take size_t as template argument
   as per LWG issue 755

   10. the non-trivial bits could use some comments :)

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?

A few notes on style :)

   a) please re-read points (0), (1), and (5) here:
   http://www.nabble.com/forum/ViewPost.jtp?post=18174685

   b) please use the name _TypeT (as opposed to _Type) for template
   parameters

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

Martin

> 
> Brad.
> 
>> -----Original Message-----
>> From: Eric Lemings [mailto:Eric.Lemings@roguewave.com] 
>> Sent: Monday, June 30, 2008 4:42 PM
>> To: dev@stdcxx.apache.org
>> Subject: RE: Tuple status
>>
>>  
>> Committed.
>>
>> Note, there is still a problem with using reference wrappers with
>> make_tuple() which I'm currently working on but I didn't want 
>> to hold up
>> this rather large change any longer.
>>
>> Brad.
>>
>>> -----Original Message-----
>>> From: Eric Lemings [mailto:Eric.Lemings@roguewave.com] 
>>> Sent: Monday, June 30, 2008 10:46 AM
>>> To: dev@stdcxx.apache.org
>>> Subject: Tuple status
>>>
>>>  
>>> Just a brief status on tuple progress.
>>>  
>>> I got the remaining portions of tuple (except the tie() 
>> function) work
>>> late Friday.  I did a personal code review over the weekend and am
>>> applying some cleanup and other finishing touches.  Should 
>> be checking
>>> in a lot of changes later today.  So just a heads up.
>>>  
>>> Brad.
>>>


Mime
View raw message