4
\$\begingroup\$

Anonymous functions are one of the best features of C++11. They make everything so beautiful!

However, one can get carried away and start overusing them.

This code calls a function that reads through a file and invokes a callback everytime that something matches a regex. Since I read two different files but I want callbacks only to differ for a constant, I chose this "lambda-returning-lambda" factory pattern.

auto callback_factory = 
    [&] (IPMarkerType start, IPMarkerType stop) -> std::function<void(IPNode<T>&)>
{
    return [&,start,stop](IPNode<T> & node){ 
        markers.push_back(IPMarker<T>(node.ip, start));  
        markers.push_back(IPMarker<T>(node.ip.network_ones(node.prefix), stop));
    };
};

read_regexp<T>("C:\\path\\to\\file1.txt", regex, callback_factory(ipm_a_open,ipm_a_close));
read_regexp<T>("C:\\path\\to\\file2.txt", regex, callback_factory(ipm_b_open,ipm_b_close));

Callbacks are really local just to the calling function, so I think that external functor would not be justified. On the other hand, this code smells a lot to me, so I would like to hear your comments.

\$\endgroup\$
3
  • \$\begingroup\$ Meh, looks good to me. Just make the node argument const&. \$\endgroup\$ Commented Aug 13, 2012 at 11:50
  • \$\begingroup\$ Any reason you need to declare the return type of the outer lambda? \$\endgroup\$
    – ecatmur
    Commented Aug 13, 2012 at 11:55
  • \$\begingroup\$ @ecatmur Visual Studio 11 wouldn't compile otherwise. \$\endgroup\$
    – user13049
    Commented Aug 13, 2012 at 17:15

2 Answers 2

5
\$\begingroup\$

First of all, I will assume there is something like template <typename T> somewhere in your code. Otherwise, I doubt it would compile.

There is something you could do to improve your program: use emplace_back instead of push_back with an object construction. That way, objects will be created right in the collection (list, vector, etc...). By the way, I'll also assume that markers is a standard collection.

auto callback_factory = 
    [&] (IPMarkerType start, IPMarkerType stop) -> std::function<void(IPNode<T>&)>
{
    return [&,start,stop](IPNode<T> & node){ 
        markers.emplace_back(node.ip, start);  
        markers.emplace_back(node.ip.network_ones(node.prefix), stop);
    };
};

And, yeah, something still smells: I really wonder how and/or where you declared markers. If it's a global variable, then I think the template parameter may cause some problems.

\$\endgroup\$
1
  • \$\begingroup\$ I think it’s pretty obvious that this code is just part of a function, which obviates your first and last paragraph. \$\endgroup\$ Commented Aug 13, 2012 at 12:52
1
\$\begingroup\$

I suspect I'd hoist the start/stop parameters out, something like this:

template <typename T, typename Markers>
void regex_start_stop_wrapper(char const *path, char const *regex,
                              Markers &markers,
                              IPMarkerType start, IPMarkerType stop)
{
    read_regexp<T>(path, regex,
                   [&markers,start,stop](IPNode<T> & node)
                   { 
                       markers.push_back(IPMarker<T>(node.ip, start));  
                       markers.push_back(IPMarker<T>(node.ip.network_ones(node.prefix), stop));
                   }
                   );
}

void test()
{
    // ... original call sites now look like this:
    regex_start_stop_wrapper<T>("C:\\path\\to\\file1.txt", regex,
                                markers, ipm_a_open, ipm_a_close);
    regex_start_stop_wrapper<T>("C:\\path\\to\\file2.txt", regex,
                                markers, ipm_b_open, ipm_b_close);
}

It just seems like a simpler interaction between the callback, the call site and the read_regexp, somehow. I think it's because this is a straighter call chain, compared to the factory-lambda-returning-a-nested-lambda approach, which is a bit like the world's densest dependency injection.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.