Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am designing a class which (among other things) acts as a vertex in an undirected graph.

#include <iterator>
#include <string>

class Node
{
public:
    explicit Node(const std::string& name);
    Node(const Node&);
    Node(Node&&);

    class iterator
        : public std::iterator<std::random_access_iterator_tag, const Node>
    {
        //...
    };
    class container_proxy
    {
    public:
        iterator begin() const;
        iterator end() const;
    private:
        //...
    };

    container_proxy neighbors() const;

    add_link(Node& other);

    //...

private:
    //...
};
// to support: for (const Node& neighbor : node.neighbors())
Node::iterator begin(const Node::container_proxy&);
Node::iterator end(const Node::container_proxy&);

Now I want to define a function link_complete which:

  • takes any number of arguments
  • allows non-const Node lvalues (parameter type Node&)
  • allows non-const Node rvalues (parameter type Node&&)
  • does not allow any other parameter type.

An example usage:

Node ret_by_value();

void example_test(Node& x, Node&& y)
{
    link_complete(x, ret_by_value(), std::move(y), Node("test"));
}

Here's the solution I came up with:

#include <initializer_list>
#include <type_traits>

class Node {
    //...
public:
    static void link_complete(std::initializer_list<Node*> node_list);
};

template <typename ... Types>
struct template_all; // UNDEFINED

template <>
struct template_all<>
  : public std::true_type {};

template <typename ... Types>
struct template_all<std::false_type, Types...>
  : public std::false_type {};

template <typename ... Types>
struct template_all<std::true_type, Types...>
  : public template_all<Types...>::type {};

template <typename ... Args>
auto link_complete(Args&& ... args) ->
  typename std::enable_if<
    template_all<
      typename std::is_same<
        typename std::remove_reference<Args>::type,
        Node
      >::type ...
    >::value,
    void
  >::type
{
    Node::link_complete( { &args... } );
}

It seems to work as required, but I'm wondering if there was a simpler or "prettier" way to do it, or maybe a way to improve that template_all helper type. All C++11, TR1, and Boost features are welcome. I did notice boost::mpl::and_ looks similar to my template_all helper type, but Boost MPL seems like a tricky learning curve and/or overkill for this problem.

share|improve this question
add comment

1 Answer

up vote 5 down vote accepted
+50

Do not inherit from std::iterator

I know that we are not supposed to review the first piece of code with the Node class, but I still have something to say about it (I'm being nitpicky): when you write an iterator class, you should't inherit from std::iterator. Sorry for the link to one of my own answers, but I did some research and the standard guys now think that inheriting from std::iterator is not a good idea. One of the reasons is that it increases coupling and that we don't gain much from it. You should write the typedef in your iterator class instead.

EDIT: while I probably picked good link to prove my point, the recent comments prove that this opinion is actually mitigated. You are free to trust whoever you want :)

struct has public inheritance

By default, struct has public inheritance. Therefore, you don't need to manually specify it every time. You can write shorter (and so more readable) code:

template <>
struct template_all<>
  : std::true_type {};

template <typename ... Types>
struct template_all<std::false_type, Types...>
  : std::false_type {};

template <typename ... Types>
struct template_all<std::true_type, Types...>
  : template_all<Types...>::type {};

Since type traits generally use many templates, you don't want to add even more extra keywords all over the place.

Use alias templates

If you have acces to a compiler that supports some C++14 features, you will probably want to use the alias templates for transformation traits to get rid of many typename and ::type. Unfortunately, the alias templates for the query traits (is_*) have not been accepted, so you have to use std::is_same.

template <typename ... Args>
auto link_complete(Args&& ... args) ->
  std::enable_if_t<
    template_all<
      typename std::is_same<
        std::remove_reference_t<Args>,
        Node
      >...
    >::value,
    void
  >
{
    Node::link_complete( { &args... } );
}

static_assert is great

Instead of using SFINAE for link_complete, you should use static_assert since your function does not have any overload. It will allow you to give a meaningful error message instead of a somewhat obscure SFINAE error message:

template <typename ... Args>
void link_complete(Args&& ... args)
{

    static_assert(
        template_all<
            typename std::is_same<
                std::remove_reference_t<Args>,
                Node
            >::type...
        >::value,
        "link_complete arguments must be of the type Node"
    );

    Node::link_complete( { &args... } );
}

You may want to use std::decay

std::decay calls std::remove_reference and std::remove_cv internally. If needed, it also calls std::remove_extent if T is an array type and calls std::add_pointer if T is a function type. In short:

This is the type conversion applied to all function arguments when passed by value.

That allows you to check for what we generally think of as "same types":

template <typename ... Args>
void link_complete(Args&& ... args)
{

    static_assert(
        template_all<
            typename std::is_same<
                std::decay_t<Args>,
                Node
            >::type...
        >::value,
        "link_complete arguments must be of the type Node"
    );

    Node::link_complete( { &args... } );
}

Rethink template_all

All in all, your goal is to test boolean conditions on types. Since the query traits have a static constexpr bool value member, you may want to create a template_all that checks for true and false (like std::enable_if or std::conditional) instead of checking for std::true_type and std::false_type. That would make your intent clearer, but it would be more like a all_true template.

template <bool...>
struct all_true; // UNDEFINED

template <>
struct all_true<>
  : std::true_type {};

template <bool... Conds>
struct all_true<false, Conds...>
  : std::false_type {};

template <bool... Conds>
struct all_true<true, Conds...>
  : all_true<Conds...> {};

template <typename ... Args>
void link_complete(Args&& ...)
{
    static_assert(
        all_true<
            std::is_same<
                std::decay_t<Args>,
                Node
            >::value...
        >::value,
        "must have Node types"
    );

    Node::link_complete( { &args... } );
}
share|improve this answer
    
Thanks for the detailed look. I specifically don't want to remove_cv in this case. Omitting base class access rubs my style-meter the wrong way, but that's mostly preference. I like all_true (and can't really remember why I thought using type parameters was a good idea at the time). –  aschepler May 29 at 18:54
    
@aschepler I tend to consider that class or struct is enough to document public/private access, but I have to admit that beginners simply don't know the difference, so explicitly specifying it isn't bad either ^^ –  Morwenn May 29 at 18:57
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.