incubator-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: 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 21:01:39 GMT
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! :)

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

> +class tuple< _HeadT, _TailT... >
> +    : tuple< _TailT... >
> +{
> +    typedef tuple< _TailT... >  _Base;
> +
> +protected:
> +
> +    _HeadT  _C_head;

Why protected and not private?

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

> +
> +    /**
> +     * 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?

> +        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?
[...]
> 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.

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

> +
[...]
> +// 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.

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

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

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

Martin

Mime
View raw message