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: svn commit: r668318 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/rw/_tuple_traits.h include/tuple tests/utilities/20.tuple.cnstr.cpp
Date Mon, 23 Jun 2008 22:13:51 GMT
 

> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com] 
> Sent: Monday, June 23, 2008 3:02 PM
> To: dev@stdcxx.apache.org
> Subject: Re: svn commit: r668318 - in /stdcxx/branches/4.3.x: 
> include/rw/_tuple.h include/rw/_tuple_traits.h include/tuple 
> tests/utilities/20.tuple.cnstr.cpp
> 
> elemings@apache.org wrote:
> > Author: elemings
> > Date: Mon Jun 16 14:16:48 2008
> > New Revision: 668318
> > 
> > URL: http://svn.apache.org/viewvc?rev=668318&view=rev
> > Log:
> > 2008-06-16  Eric Lemings <eric.lemings@roguewave.com>
> > 
> > 	STDCXX-958
> > 	* include/tuple, include/rw/_tuple.h, 
> include/rw/_tuple_traits.h:
> > 	Add initial version of headers defining tuple interface and
> > 	implementation.  (Only tested on Linux/gcc-4.3 
> platforms so far.)
> > 	* tests/utilities/20.tuple.cnstr.cpp: Rough outline of first
> > 	tuple test program.
> > 
> [...]
> > +#    if !defined _RWSTD_NO_VARIADIC_TEMPLATES
> > +
> > +
> > +_RWSTD_NAMESPACE (std) {
> > +
> > +
> > +// 20.3.1, class template tuple:
> > +
> > +/**
> > + * @defgroup tuple Tuples [tuple]
> > + *
> > + * A fixed-size collection of values with variable, 
> heterogenous types.
> > + * This class template is used to instantatiate tuple 
> types.  A tuple
> > + * type specifies zero or more element types for defining 
> tuple values.
> > + * A tuple value can be constructed from a tuple type that 
> holds one
> > + * value for each element type in the tuple.
> > + *
> > + * @param Types A list of zero or more types.  No applicable type
> > + *              requirements or restrictions.
> > + */
> > +template < class... Types >
> 
> Please follow the established convention and drop the spaces
> before class and before the closing pointy brace. Thanks! :)

It would be a lot easier if these conventions were actually ESTABLISHED.
Until they are established, we're going to be constantly running into
this problem.  Don't you agree?

I can start a Wiki page for this unless you'd prefer to do it.

> 
> > +class tuple;
> > +
> > +/**
> > + * @internal
> > + * The template specialization for empty tuples.
> > + */
> > +_RWSTD_SPECIALIZED_CLASS
> > +class tuple< >
> 
> Also please remove the space between the pointy braces here.
> 
> > +{
> > +    // empty
> > +};
> > +
> > +/**
> > + * @internal
> > + * The basic template specialization for most tuples.  This class
> > + * template is used to instantiate all tuple types except for empty
> > + * tuples and tuples with exactly two element types.
> > + *
> > + * @param _HeadT First element type (required).
> > + * @param _TailT Template parameter pack for remaining 
> element types.
> > + */
> > +template < class _HeadT, class... _TailT >
> 
> In the interest of consistency we should adopt the same naming
> convention for the names of variadic template parameters. We
> have _TypeT and and Types in traits, _HeadT and _TailT here,
> and TTypes and UTypes in the standard. Which one should it
> be?

It is my opinion that the T and U prefixes/suffixes are only necessary
in binary contexts; i.e. where there are two different types or type
lists.  Otherwise, we should use just a plain "Type" name.

Furthermore, generic "Type" names should only be used in truly
generic contexts; i.e. contexts where just about any type can be
used.  Otherwise, the name should imply the restrictions/requirements
on the type; e.g. _ScalarType, _IntConst.  In the recursive tuple
case, using generic names would make the code much harder to follow
IMHO.


> 
> > +class tuple< _HeadT, _TailT... >
> > +    : tuple< _TailT... >
> > +{
> > +    typedef tuple< _TailT... >  _Base;
> > +
> > +protected:
> > +
> > +    _HeadT  _C_head;
> 
> Why protected and not private?

Good point.  Given there's an internal accessor now, this could be
private I think.

> 
> > +
> > +public:
> > +
> > +    /**
> > +     * Construct tuple with default values.
> > +     */
> > +    tuple ()
> > +        : _C_head (), _Base () { /* empty */ }
> 
> Unless tuple is somehow different from other (non-variadic
> classes) I think _Base() here should be listed first, because
> it's initialized first, no?

Yep.  I think these were all done in a subsequent change.

> 
> > +
> > +    /**
> > +     * Copy construct tuple from a different tuple value.
> > +     * @param __tuple Another tuple value with same type.
> > +     */
> > +    tuple (const tuple& __tuple)
> > +        : _C_head (__tuple._C_head), _Base (__tuple) { /* 
> empty */ }
> 
> Same as above.
> 
> > +
> > +    /**
> > +     * Copy assign tuple from a different tuple value.
> > +     * @param __tuple Another tuple value with same type.
> > +     */
> > +    tuple& operator= (const tuple& __tuple) {
> > +        _C_head = __tuple._C_head;
> > +        _Base::operator= (__tuple);
> 
> Similarly to the ctor, the base assignment operator should
> probably be invoked first.
> 
> > +        return *this;
> > +    }
> > +
> > +    _EXPLICIT
> > +    tuple (const _HeadT& __head, const _TailT&... __tail)
> > +        : _C_head (__head), _Base (__tail...) { /* empty */ }
> > +
> > +#      if !defined _RWSTD_NO_RVALUE_REFERENCES
> > +
> > +    tuple (tuple&& __tuple)
> > +        : _C_head (std::move (__tuple._C_head))
> > +        , _Base (std::forward<_Base> (__tuple)) { /* empty */ }
> 
> The initialization order should follow the default ctor case
> (i.e., _Base first). Also, the existing convention doesn't
> drop the comma.
> 
> > +
> > +    tuple& operator= (tuple&& __tuple) {
> > +        _C_head = std::move (__tuple._C_head);
> > +        _Base::operator= (__tuple);
> 
> Same as the other assignment operator.
> 
> Also, I think it would make the class easier to read if
> overloads were kept together wherever possible (i.e., all
> ctors, all assignment operators, etc.) Although I think
> I see why you didn't -- to avoid duplicating the rvalue
> references test. Hmm. Have we decided on the list of
> required compiler features yet? I saw your Compiler
> Requirements page on the wiki (thanks for starting it,
> btw.) and rvalue references isn't there. Do we think we
> can provide a useful implementation of C++ 0x without
> rvalue references?

No, probably not.  We could make _RWSTD_EXT_CXX_0X and
_RWSTD_NO_RVALUE_REFERENCES mutually dependent.

> 
> > +        return *this;
> > +    }
> > +
> > +#      endif   // !defined _RWSTD_NO_RVALUE_REFERENCES
> > +
> > +#      if !defined _RWSTD_NO_MEMBER_TEMPLATES
> 
> I think we can safely rely on member templates in C++ 0x.
> Do we all agree?

Yeah, I believe I removed this in a latter change.

> [...]
> > Added: stdcxx/branches/4.3.x/include/rw/_tuple_traits.h
> [...]
> > +template <class _Type>
> > +struct constructible_with_allocator_prefix;
> > +
> > +/**
> > + * Allows tuple construction with an allocator prefix 
> argument.  This
> > + * trait informs other library components that tuple can 
> be constructed
> > + * with an allocator prefix argument.
>                          ^^^^^^^^
> 
> Looks like an unreadable character has sneaked in here.

Noted.

> 
> [...]
> > Added: stdcxx/branches/4.3.x/include/tuple
> [...]
> > +
> > +#if defined _RWSTD_NO_EXT_CXX_0X
> > +#  error _RWSTD_NO_EXT_CXX_0X defined and C++0x header included
> > +#endif   // defined _RWSTD_NO_EXT_CXX_0X
> > +
> > +#ifndef _RWSTD_TUPLE_INCLUDED
> > +#  define _RWSTD_TUPLE_INCLUDED
> > +
> > +#  include <rw/_tuple.h>
> > +#  include <rw/_tuple_traits.h>
> > +
> > +
> > +_RWSTD_NAMESPACE (__rw) {
> > +
> > +struct __rw_ignore { /* empty */ };
> > +
> > +/// Transforms _Type into a suitable make_tuple() return type.
> > +template <class _Type>
> > +struct __rw_make_tuple {
> > +    /// @todo Deduce correct return type.
> > +    typedef _Type type;
> > +};
> > +
> > +}   // namespace __rw
> > +
> > +
> > +_RWSTD_NAMESPACE (std) {
> > +
> > +
> > +// 20.3.3, tuple creation functions:
> > +
> > +const _RW::__rw_ignore ignore = _RW::__rw_ignore ();
> 
> This either needs to be static or preferably defined out
> of line (in a .cpp file). Ditto for std::allocator_arg
> in rw/_allocator.h.

Yep.  Will rectify.

> 
> > +
> [...]
> > +// 20.3.1.4, tuple helper classes:
> > +
> > +template <class>
> 
> Please don't omit the names of template parameters. I wish
> we could but I don't think aCC 3 supports it yet.

Ah.  Wasn't aware of that.

> 
> > +class tuple_size; // undefined
> > +
> > +template <class... _Types>
> > +class tuple_size<tuple<_Types...> >;
> > +
> > +template <int, class>
> > +class tuple_element; // undefined
> > +
> > +template <int _Index, class... _Types>
> > +class tuple_element<_Index, tuple<_Types...> >;
> > +
> > +
> > +// 20.3.1.5, element access:
> > +
> > +template <int _Index, class... _Types>
> > +_TYPENAME tuple_element<_Index, tuple<_Types...> >::type&
> > +get (tuple<_Types...>&);
> > +
> > +template <int _Index, class ... _Types>
> > +_TYPENAME tuple_element<_Index, tuple<_Types...> >::type const&
> 
> The const should come before tuple_element.

For style?  They are semantically equivalent though, aren't they?
BTW, this is how it is specified in the draft.

> 
> > +get (const tuple<_Types...>&);
> > +
> > +
> > +// 20.3.1.6, relational operators:
> > +
> > +template <class... _TypesT, class... _TypesU>
> > +bool operator== (const tuple<_TypesT...>& __x,
> > +                 const tuple<_TypesU...>& __y);
> 
> I think aCC 6 gives a remark for function argument names
> in function prototypes.

That's a placeholder.  Once I implement it, the names will be
required.

> 
> > +
> > +template <class... _TypesT, class... _TypesU>
> > +bool operator< (const tuple<_TypesT...>& __x,
> > +                const tuple<_TypesU...>& __y);
> > +
> > +/**
> > + * @param __x Tuple value on LHS of operator.
> > + * @param __y Tuple value on RHS of operator.
> > + * @return !(__x == __y)
> > + */
> > +template <class... _TypesT, class... _TypesU>
> > +bool operator!= (const tuple<_TypesT...>& __x,
> > +                 const tuple<_TypesU...>& __y)
> > +{
> > +    return !(__x == __y);
> > +}
> 
> This and all other function templates defined here need to
> be declared inline.

Yep.  I was wondering about that today and found pretty uniform
`inline' usage.

Brad.

Mime
View raw message