Sign in to follow this  

Should I rewrite my code to use STD algorithm?

This topic is 3667 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I understand from pass post that it is benifical to use the std algorithm. However I already have a number of files that does not use the std algorithm like std::for_each(); My existing code are like this
for (int i=0; i<(int)MyVector.size(); i++)
    SAFE_RELEASE(MyVector[i]);

or it may be slightly more complicated in the for loop, like orthogonalize-ing a list/vector of vertices. My problem, is for such code, should i rewrite them using std::for_each(), where I have to write another functor(). Especially in cases where it is just a SAFE_RELEASE line/macro in the for loop, writing a functor to excute that seems a little overkill. Or should I just go ahead and convert my code?

Share this post


Link to post
Share on other sites
What's the goal of refactoring?

Does it work? Would a for_each make it faster?

If you're answer is yes to the first, and no to the second, then you have no good reason to change it. While programming is abstract, and somewhat complex, you're functions work if they work. If they don't, they don't. There's a large pragmatism to it too.

If you're sharing your code with other programmers, and the group would like to use STD library functions and structures as much as possible, then change it. That makes your team more efficient, as they won't be tripping over the reason why your code is shaped that way each time they read it.

"If it ain't broke, don't fix it."

Share this post


Link to post
Share on other sites
A couple of comments/questions:
for (int i=0; i<(int)MyVector.size(); i++)
SAFE_RELEASE(MyVector[i]);
What does SAFE_RELEASE do? Is it a macro? Does MyVector store pointers?

If MyVector does store pointers, then you could avoid manual cleanup entirely by switching to smart pointers. Sticking with the original example though, you could substitute a free 'deleter' function for SAFE_RELEASE and write:
std::for_each(MyVector.begin(), MyVector.end(), &Delete);
Performance will likely be the same either way, but I find the for_each() version to be clearer and more expressive.

I generally use for_each() when it's fairly straightforward and easy to do so. If however it would require creating some elaborate function object, I just write out the loop manually (but usually using iterators, not indices).

But of course, YMMV.

Share this post


Link to post
Share on other sites
In most situations, I would move to std::for_each. In the situation you outline, however, I would probably use destructors and clear() instead, judging from the semantics you imply.

Share this post


Link to post
Share on other sites
Last I checked, the entire purpose of using the standard library functions in the first place is to use a good and solid version of algorithms that you don't have to spend the time associated with writing and testing. Thus if the time to create your own things has already been spent, there is really no point in spending even more time to migrate to a set of functions that is designed to save you time. Seems kinda backward.

Share this post


Link to post
Share on other sites
It looks like I'd rewrite your code to use boost's ptr_vector (!).

To say that macros play poorly with function adapters is an understatement. I'm curious as to what SAFE_RELEASE actually does, in that chances are I'd write it as a function of some sort instead of as a macro.

And, while I probably wouldn't bother to go around rewriting all my loops, I would use BOOST_FOREACH, especially in any situation where you'd end up writing out iterator types, over hand rolled loops where possible.

Or. You know. Stop using C++ when I can. But that's a whole 'nother issue.

Share this post


Link to post
Share on other sites
Quote:

It looks like I'd rewrite your code to use boost's ptr_vector (!).

I wouldn't.

Yeah, that's right, I just totally disagreed with you! WHAT NOW?

littlekid:
The primary benefit you'd gain from refactoring your for loop into an iterator-based one, or one based on the various "for each" implementations available, would be transparency with respect to the underlying container. For example, you could change "MyVector" to a std::list, and the code would not break (as it stands now, it will, since a std::list and indeed many other SC++L containers cannot be indexed as a vector can).

However (and this is why I disagree with MaulingMonkey's usually spot-on advice), SAFE_RELEASE implies to me the typical "if(ptr != 0) { ptr->Release(); ptr = 0; }" you so often see with COM interfaces such as those one uses when dealing with DirectX. In terms of refactoring bang-for-your-buck, you'll get the most out of using the CComPtr<T> interface to hold those objects you are currently "safe releasing." CComPtr<T> is a COM smart pointer that will handle the calling of the AddRef() and Release() functions for you, using RAII, so you will no longer need to worry about these sorts of "release loops."

I could be wrong (I don't use ptr_vector or the Boost pointer containers much), but my understanding of them is that they (a) clone the objects inserted and (b) provide no "custom deleter" access like the Boost smart pointers do. Which means you would still have to manually be managing the Release() and potentially AddRef() calls (depending on what you can do with the ptr_vector's CloneAllocator) carefully.

Basically, if you refactor your code to use CComPtr<T>, you can actually remove all such loops from your code, which is better than simply rewriting all such loops to use some slightly modified looping paradigm.

Share this post


Link to post
Share on other sites
How do i exactly use the setup to use the ATL CComPtr?

I added in this header file <atlcomcli.h>, compile and it says GUID_NULL is an unidentified identifier

Must I add the specific atlcomcli etc header for each atl class that i will use? Or is there a generic one that add all of them. I know msdn have a list of atl class listed here:

http://msdn2.microsoft.com/en-us/library/awt7k7f5(VS.71).aspx

Just wondering if there is a single include file that contains all, just being lazy to type :)

Share this post


Link to post
Share on other sites

This topic is 3667 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this