stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp
Date Wed, 09 Jul 2008 17:09:54 GMT
Travis Vitek wrote:
>  
> 
>> Author: elemings
>> Date: Tue Jul  8 16:13:36 2008
>> New Revision: 675044
>>
>> URL: http://svn.apache.org/viewvc?rev=675044&view=rev
>> Log:
>> 2008-07-08  Eric Lemings <eric.lemings@roguewave.com>
>>
>> 	STDCXX-958
>> 	* include/tuple: Second parameter in value move ctor of pair
>> 	specialization missing rvalue reference.
>> 	(make_tuple, get, relational operators): Explicitly declare
>> 	as inline functions.
>> 	(tie): Implemented.
>> 	* include/rw/_tuple.h: Fix move semantics in heterogenous move
>> 	assignment operator.
>> 	(__rw_ignore): Add assignment operator to ignore all values.
>> 	* tests/utilities/20.tuple.cnstr.cpp: Added V&V for tuple
>> 	state and invariants.  Manually inspected proper construction
>> 	of all test tuples.  Updated/corrected/added tests as necessary.
>> 	* tests/utilities/20.tuple.creation.cpp: Added simple tie()
>> 	test.
>> 	* tests/utilities/20.tuple.h: Minor stylistic changes.
>> 	* tests/utilities/20.tuple.helpers.cpp: Same.
>>
>>
>> Modified:
>>    stdcxx/branches/4.3.x/include/rw/_tuple.h
>>    stdcxx/branches/4.3.x/include/tuple
>>    stdcxx/branches/4.3.x/tests/utilities/20.tuple.cnstr.cpp
>>    stdcxx/branches/4.3.x/tests/utilities/20.tuple.creation.cpp
>>    stdcxx/branches/4.3.x/tests/utilities/20.tuple.h
>>    stdcxx/branches/4.3.x/tests/utilities/20.tuple.helpers.cpp
>>
>> Modified: stdcxx/branches/4.3.x/include/rw/_tuple.h
>> URL: 
>> http://svn.apache.org/viewvc/stdcxx/branches/4.3.x/include/rw/_
>> tuple.h?rev=675044&r1=675043&r2=675044&view=diff
>> ===============================================================
>> ===============
>> --- stdcxx/branches/4.3.x/include/rw/_tuple.h (original)
>> +++ stdcxx/branches/4.3.x/include/rw/_tuple.h Tue Jul  8 16:13:36 2008
>> @@ -174,7 +174,14 @@
>> };
>>
>>
>> -struct __rw_ignore { /* empty */ };
>> +struct __rw_ignore
>> +{
>> +    template <class _TypeT>
>> +    inline __rw_ignore const&
>> +    operator= (const _TypeT& /*unused*/) const {
>> +        return *this;
>> +    }
>> +};
>>
> 
> I think the commented out parameter name should be removed. I don't see
> this in existing code, and I personally find it a bit distracting.

I agree. Without a name, it's obvious that the parameter
is unused. Saying it's unused in a comment is like adding
a /* return; */ comment to the end of void functions, or
adding an /* extern */ in front of the definition of non
member function definitions. IMO, all of these represent
unnecessary redundancies that are liable to make readers
wonder about their purpose rather than providing any
helpful insight. It would be much more helpful to document
the purpose of the unusual assignment operator than the
unused argument :)

> 
> This can probably be changed to use a void return type, which will
> simplify the code further. You only really need the return type to chain
> assignments or to call a function on the result, none of which we should
> be doing.

Good idea! Also, the inline specifier is redundant and should
be removed.

> 
[...]
>> @@ -377,7 +381,7 @@
>> // 20.3.1.5, element access:
>>
>> template <_RWSTD_SIZE_T _Index, class _Head, class... _Tail>
>> -_TYPENAME tuple_element<_Index, tuple<_Head, _Tail...> >::_Ref
>> +inline _TYPENAME tuple_element<_Index, tuple<_Head, _Tail...> >::_Ref
>> get (tuple<_Head, _Tail...>& __tuple)
>> {
>>     typedef tuple_element<_Index, tuple<_Head, _Tail...> > _Tuple;
> 
> In the recent past Martin recommended not using the _TYPENAME macro. It
> isn't hurting anything, but it could probably be removed. I know that I
> no longer use it in the traits code. I also noticed the _EXPLICIT macro
> above. I think that one should be added to the list. Martin?

I agree. We can start using typename and explicit in all new
code on 4.3.x and replace _TYPENAME and _EXPLICIT with the
real keywords. Ditto for _RWSTD_SPECIALIZED_FUNCTION and
_RWSTD_SPECIALIZED_CLASS.

> 
[...]
>> @@ -396,59 +400,67 @@
>> // 20.3.1.6, relational operators:
>>
>> template <class... _TypesT, class... _TypesU>
>> -bool operator== (const tuple<_TypesT...>& __x,
>> -                 const tuple<_TypesU...>& __y)
>> +inline bool
>> +operator== (const tuple<_TypesT...>& __x,
>> +            const tuple<_TypesU...>& __y)
>> {
>>     return _RWSTD_STATIC_CAST (const 
>> _RW::__rw_tuple<_TypesT...>&, __x)
>>            == _RWSTD_STATIC_CAST (const 
>> _RW::__rw_tuple<_TypesU...>&, __y);
> 
> I think there is a formatting issue here. The prevailing style is for
> the operator to start of the next line, but for the operands to be lined
> up on their left. As an example
> 
>   return    __some_really_long_expression_1
>          == __some_really_long_expression_2;

Right. We're not 100% consistent (and I can't say I'm crazy
about this style, either) but I find it more readable than
any of the alternatives I've seen.

Martin


Mime
View raw message