Jump to content
  • Advertisement
Sign in to follow this  
littlekid

Should I rewrite my code to use STD algorithm?

This topic is 3974 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);

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
Advertisement
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);
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
The documentation here indicates the header is atlbase.h. That should be all you need. If you're using Direct3D, you should be aware that using a CComPtr for the device object itself requires some trickery in D3D9. See here.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!