Skip to main content
GameDev.net gamedev.net
🔒 Locked

x.begin(), x.end()

Started by DevFred Jul 10, 2008 at 1:14 PM 20 replies 2.9k views
Original Post
DevFred
DevFred
When using STL algorithms, I often find myself typing stuff like 

accumulate(a.begin(), a.end(), 0);
for_each(b.begin(), b.end(), foo);
remove_if(c.begin(), c.end(), bar);
Today I thought to myself: why not use the preprocessor to make things clearer? 

#define ALL(x) x.begin(), x.end()

accumulate(ALL(a), 0);
for_each(ALL(b), foo);
remove_if(ALL(c), bar);
As long as x is just a variable, I don't see any problems with it. Is this a reasonable use of the evil preprocessor? Maybe there's even a real solution I don't know about? Discuss!
Portmanteau
Portmanteau
Let's be honest, you are making your code more unclear as well as sacrificing type safeness in order to save a little extra typing.
Promit
Promit
Using the preprocessor to obscure and de-idiomize code just because you are lazy is not generally considered to be a good idea.
SlimDX | Ventspace Blog | Twitter | Diverse teams make better games. I am currently hiring capable C++ engine developers in Baltimore, MD.
Antheus
Antheus
Quote:
Today I thought to myself: why not use the preprocessor to make things clearer?


Why not use language itself? C++ in general has no overhead for syntactic sugar, especially when dealing with templates.

template < class Container, class T >void accumulate(Container & c, T t){  accumulate(c.begin(), c.end(), t);};


accumulate(a, 0);

Or something....

dmatter
dmatter
Taking for_each(ALL(b), foo) as an example, wouldn't this be better?:

template <class InputIterableContainer, class UnaryFunction>inline UnaryFunction for_each(InputIterableContainer & c, UnaryFunction f){    return std::for_each(c.begin(), c.end(), f);}
Sneftel
Sneftel 
#define ALL(x) x.begin(), x.end()class Foo{public:   void AddValue(int i)   {      m_values.push_back(i);   }   std::vector<int> GetValues() const   {      return m_values;   }private:   std::vector<int> m_values;};void printFoo(Foo f){    std::copy(ALL(f.GetValues()), std::ostream_iterator<int>(std::cout));}

Spot the bug.


Also, check out boost::range.
Palidine
Palidine
Quote:
Original post by Sneftel
*** Source Snippet Removed ***
Spot the bug.


getValues() returns a copy & the pre-processor makes x == f.getValues() instead of just the vector, so the .begin() and the .end() are iterators from different vectors. ZOMG, what do i win? [smile]

-me
dmatter
dmatter
I believe the problem can be generalised by saying it's an ill-behaved macro because it evaluates its arguments more than once.
Rydinare
Rydinare
Quote:
Original post by dmatter
I believe the problem can be generalised by saying it's an ill-behaved macro because it evaluates its arguments more than once.


Writing macros in such a way that these issues can be avoided can be a headache, especially since at first glance, it looks just fine. The reality is that there's no reason for the riskier macro when saver solutions exist, so why even take the risk? A macro is an absolute last resort for me. Especially when you consider that to avoid clashes, a better name than "ALL" is required. You might need something like:

#define STL_ALGORITHM_HELPER_ALL(x) x.begin(), x.end()...accumulate(STL_ALGORITHM_HELPER_ALL(a), 0);


Once you do that, the supposed reduced typing of the macro is proven to be a fallacy and the macro loses on all accounts.
Zahlman
Zahlman
Quote:
Original post by Palidine
Quote:
Original post by Sneftel
*** Source Snippet Removed ***
Spot the bug.


getValues() returns a copy & the pre-processor makes x == f.getValues() instead of just the vector, so the .begin() and the .end() are iterators from different vectors. ZOMG, what do i win? [smile]

-me


Yep. You win a cookie.

The unfortunate thing from a pedagogical perspective is that changing to return-by-const-reference - which should probably be done anyway - would fix the bug (but not the design problem).
OrangyTang
OrangyTang
Quote:
Original post by Zahlman
Quote:
Original post by Palidine
Quote:
Original post by Sneftel
*** Source Snippet Removed ***
Spot the bug.


getValues() returns a copy & the pre-processor makes x == f.getValues() instead of just the vector, so the .begin() and the .end() are iterators from different vectors. ZOMG, what do i win? [smile]

-me


Yep. You win a cookie.

The unfortunate thing from a pedagogical perspective is that changing to return-by-const-reference - which should probably be done anyway - would fix the bug (but not the design problem).

I'd be more terrified by the reverse happening - the original code returning by const reference and everything working fine, then an unrelated change happens to switch to return by value and all of a sudden we've introduced a really obscure bug in an area of the code we apparently havn't touched. [tears]
Rydinare
Rydinare
The scary part is how often macros are employed. I have coworkers who tended to do the same thing. I've slowly had them using small functions and template functions where they would've used macros in the past. But it is a tough habit to break, it seems.

Of course what took the cake was when I saw an STL-like linked list class implemented as a single macro back at my prior company. Pretty brutal.
Almindor
Almindor
Nothing beats DO_THE_FUNKY_STUFF macro I saw mentioned somewhere once. I believe it had about 400 lines of code in it.
thre3dee
thre3dee
I've come up with a more general solution. I had to use a class template cause function templates wouldn't let me do this.

template <class T, class Iter = typename T::iterator>class range{public:	// function type to send to algorithm	typedef void (*Function) (typename T::reference);	// algorithm function type	typedef Function (*Algorithm) (typename Iter, typename Iter, Function);	// constructor acts like a function	inline range (Algorithm algorithm, T& container, Function function)	{		algorithm (container.begin(), container.end(), function);	}};// Prints the number sent by for_eachvoid print_nums (int& number){	printf ("%d\n", number);}int main (){	// create a vector of ints	std::vector <int> nums;	nums.push_back (1);	nums.push_back (2);	nums.push_back (3);	// call the for_each algorithm on the vector	range <std::vector <int>> (std::for_each, nums, print_nums);	return 0;}
MaulingMonkey
MaulingMonkey
For extra irony points, I'd prefer BOOST_FOREACH over that:

#include <boost/foreach.hpp>void print_nums (int& number){	printf ("%d\n", number);}int main (){	// create a vector of ints	std::vector <int> nums;	nums.push_back (1);	nums.push_back (2);	nums.push_back (3);	BOOST_FOREACH( int& number, nums ) {		print_nums(number);	}	return 0;}


Yes, BOOST_FOREACH is a macro. But no, it's not just a macro. It's implementation is worthy of it's own article. Please note the actual implementation (for portability reasons, I'm presuming) is even more complex than the article would suggest.
Telastyn
Telastyn
Quote:
Original post by Almindor
Nothing beats DO_THE_FUNKY_STUFF macro I saw mentioned somewhere once. I believe it had about 400 lines of code in it.


I'd not actually seen it, but at a company I used to work for the word was that the entirety of the intrusion detection engine was written in macro. It took about 8 hours to build and by all accounts, inscrutable. (but very, very, very fast)
Sneftel
Sneftel
Quote:
Original post by Rydinare
Of course what took the cake was when I saw an STL-like linked list class implemented as a single macro back at my prior company. Pretty brutal.

That's long been a common C idiom, actually. Gives tighter code than void*, and actual type-safety. Terrifying to have to work with, of course. The existence of that idiom is pretty much the reason templates exist.
Zahlman
Zahlman
Quote:
Original post by Telastyn
I'd not actually seen it, but at a company I used to work for the word was that the entirety of the intrusion detection engine was written in macro. It took about 8 hours to build and by all accounts, inscrutable. (but very, very, very fast)


"intrusion detection engine"?

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.