Sign in to follow this  

Help! I don't know if I'm doing this STL removing correctly

This topic is 4356 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

typedef shared_ptr<PendingOpenFile> PendingOpenFilePtr;

void PtDb::TryOpenPendingFiles() {
	for(PendingOpenFileList::iterator iter = m_PendingFiles.begin();
		iter != m_PendingFiles.end(); iter++)
	{
		PendingOpenFilePtr pending_file = *iter;

		DicomUid ptuid;
		PtFilePtr patient_file(new PtFile);
		bool removefile = false;

		patient_file->m_File = pending_file->m_File;

		if(OpenPtFile(patient_file, ptuid))
			if(ProcessNewPtFile(patient_file, ptuid))
				removefile = true;

		if(removefile || (++pending_file->m_Try > 3)) {
			// finally, remove file from list and bail if we are done
			iter = m_PendingFiles.erase(iter); /// **** <--- THIS ****
			if(iter == m_PendingFiles.end())
				break;
		}
	}
}
Am I doing this right? I want to go through every PendingOpenFileList item, and remove it from the list if the condition if(removefile || (++pending_file->m_Try > 3)) is met - inside the for loop, and keep on going until there are no items left in the vector. Thanks!

Share this post


Link to post
Share on other sites
No, that won't work because erase() invalidates the iterator you are using to iterate through the list. Basically that means there are two ways to handle it - either you have to use an integer index into the vector and only increment when you don't destroy the previous element, or use std::remove_if and then shrink the vector afterwards.

Share this post


Link to post
Share on other sites
Quote:
Original post by ZQJ
No, that won't work because erase() invalidates the iterator you are using to iterate through the list. Basically that means there are two ways to handle it - either you have to use an integer index into the vector and only increment when you don't destroy the previous element, or use std::remove_if and then shrink the vector afterwards.


Or store the next iterator in the beginning and set the erased iterator to point to that instead [wink]. Of course that extra iter++ as per the for loop will mess that idea up, but that can be resolved.


typedef shared_ptr<PendingOpenFile> PendingOpenFilePtr;

void PtDb::TryOpenPendingFiles()
{
for(PendingOpenFileList::iterator iter = m_PendingFiles.begin();
iter != m_PendingFiles.end(); )
{
// the next iterator
myNextItr = (iter + 1);
PendingOpenFilePtr pending_file = *iter;

DicomUid ptuid;
PtFilePtr patient_file(new PtFile);
bool removefile = false;

patient_file->m_File = pending_file->m_File;

if(OpenPtFile(patient_file, ptuid))
if(ProcessNewPtFile(patient_file, ptuid))
removefile = true;

if(removefile || (++pending_file->m_Try > 3))
{
// finally, remove file from list and bail if we are done
m_PendingFiles.erase(iter); /// **** <--- THIS ****
iter = myNextItr;
if(iter == m_PendingFiles.end())
break;
}
else
{
// if we get here, nothing was removed so we can just increment. Also note that when we do remove, we dont want to increment, so this takes care of that as well.
iter++;
}

}
}



That isn't tested for obvious reasons, but it should work. (I hope)

Share this post


Link to post
Share on other sites
Actually, Washu from #gamedev helped me on this one already. Here's the code I use now:
void PtDb::TryOpenPendingFiles() {
// try to open all files, and make them "dead" if they load if or
// if ++PendingOpenFile::m_Try > 3
for(PendingOpenFileList::iterator files = m_PendingFiles.begin();
files != m_PendingFiles.end(); files++)
{
PendingOpenFilePtr pending_file = *files;

DicomUid ptuid;
PtFilePtr patient_file(new PtFile);

patient_file->m_File = pending_file->m_File;

if(OpenPtFile(patient_file, ptuid))
if(ProcessNewPtFile(patient_file, ptuid)) {
// don't need the pending file anymore
pending_file->m_Dead = true;
}

if(++pending_file->m_Try > 3)
pending_file->m_Dead = true;
}

// remove any dead files
files =
remove_if(m_PendingFiles.begin(), m_PendingFiles.end(),
PendingFileIsDead());
m_PendingFiles.erase(files, m_PendingFiles.end());
}


Edit: My rating went down 23 points after posting this, wtf?!

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by andreib
*** Source Snippet Removed ***
Am I doing this right? I want to go through every PendingOpenFileList item, and remove it from the list if the condition if(removefile || (++pending_file->m_Try > 3)) is met - inside the for loop, and keep on going until there are no items left in the vector.

Thanks!


"Filtering" of this sort is best done via the "erase-remove idiom". It's common enough that I'd write a function for it:


#include <functional>
#include <algorithm>
// Plus all you had before.

// Remove all x from c such that pred(x) is true. Returns c by reference,
// for chaining.
// c should be a container class which provides a .erase() member function.
// pred should be a model of Predicate as per SGI STL documentation.
template <typename Container, typename Predicate>
Container& filterOut(Container& c, Predicate pred) {
c.erase(std::remove_if(c.begin(), c.end(), pred), c.end());
}

// Similarly, remove elements where pred(x) is false. For convenience.
// Something like Python [x for x in c if pred(x)], since when pred(x) is
// true, the item is included (not removed).
template <typename Container, typename Predicate>
Container& filterTo(Container& c, Predicate pred) {
c.erase(std::remove_if(c.begin(), c.end(), std::not1(pred)), c.end());
}

// You could also write similar functions using std::remove instead of
// std::remove_if, but I don't know how you'd name them to avoid ambiguity.
// Instead, you might want to use this helper to create an appropriate
// "check for equality" predicate:
// template <class AdaptableBinaryFunction, class T>
// std::binder2nd<AdaptableBinaryFunction> equality(const T& c) {
// return std::bind2nd(equal_to<T>(), c);
// }
// I think I got that right ^^;

// Now to use all this to solve your problem...

typedef shared_ptr<PendingOpenFile> PendingOpenFilePtr;

// I'm assuming here that OpenPtFile and ProcessNewPtFile are
// free functions. If they are PtDb member functions, you should make a
// fileRemovalDecisionMaker ctor that accepts a PtDb* and stores it, and then
// uses it where appropriate - and then construct it in TryOpenPendingFiles()
// like "fileRemovalDecisionMaker(this)".
// In either case, you might consider keeping a fileRemovalDecisionMaker
// instance as a PtDb member, and/or making it an inner class. I don't know
// your codebase so I can't make all your design decisions.

class fileRemovalDecisionMaker {
bool operator() (const PendingOpenFile& pending_file) {
DicomUid ptuid;
PtFilePtr patient_file(new PtFile);

patient_file->m_File = pending_file->m_File;

return((OpenPtFile(patient_file, ptuid) && // able to open and process
ProcessNewPtFile(patient_file, ptuid)) || // or too many tries
++pending_file->m_Try > 3);
}
};

void PtDb::TryOpenPendingFiles() {
filterOut(m_PendingFiles, fileRemovalDecisionMaker());
}



BTW, what is a "Pt" anyway? It smells here a lot like something that should be a namespace instead...

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
I am Zahlman and wrote the above. For some reason the board logged me out and refuses to log me back in with my perfectly valid password.

Share this post


Link to post
Share on other sites
Correct me if I'm wrong, but I believe most STL containers return an iterator to the next valid item on invoking erase(). So if you simply want to remove an item while iterating, you could do something like this:


for (std::list<X>::iterator iter=x.begin(); iter!=x.end(); iter++) {
iter->DoStuff();
if (iter->IsDead()) iter=x.erase(iter);
}

Share this post


Link to post
Share on other sites
Quote:
Original post by Prototype
Correct me if I'm wrong, but I believe most STL containers return an iterator to the next valid item on invoking erase(). So if you simple want to remove an item while iterating, you could do something like this:


for (std::list<X>::iterator iter=x.begin(); iter!=x.end(); iter++) {
iter->DoStuff();
if (iter->IsDead()) iter=x.erase(iter);
}


I was going to do this, until these people changed my mind:

Quote:
( 19:20 X-0ut ) after: try this:
http://hstuart.dk/paste/view.aspx?id=b935303f-8d97-4d90-8d61-fbcd4691622d
( 19:20 X-0ut ) much more elegant
( 19:20 after ) hows it more elegent?
( 19:20 after ) I have to create a class, etc...
( 19:21 X-0ut ) beats doing it in a loop
( 19:21 after ) or a binary function
( 19:21 after ) it's easier and simpler to use a loop
( 19:21 darookie ) after: not at all
( 19:21 @ Washu ) not really
( 19:21 darookie ) the loop is more error-prone
( 19:21 @ Washu ) it's also more correct
( 19:21 X-0ut ) yeah go crash crash boom
( 19:21 @ Washu ) and makes your code easier to read
( 19:22 after ) urgh fine I'll change it

Share this post


Link to post
Share on other sites
Quote:
Original post by Prototype
Correct me if I'm wrong, but I believe most STL containers return an iterator to the next valid item on invoking erase(). So if you simply want to remove an item while iterating, you could do something like this:


for (std::list<X>::iterator iter=x.begin(); iter!=x.end(); iter++) {
iter->DoStuff();
if (iter->IsDead()) iter=x.erase(iter);
}


Actually, that won't work. It will skip the elements following the erased ones because the incrementing is done twice. Instead, you'll have to do this:

for (std::list<X>::iterator iter=x.begin(); iter!=x.end(); ) {
iter->DoStuff();
if (iter->IsDead()) iter=x.erase(iter);
else ++iter;
}


BTW, it seems that a surprisingly large number of people don't know that erase() returns an iterator to the successor of the erased element, and then resort to ugly and error-prone workarounds. Maybe this should be in the forum FAQ?

Share this post


Link to post
Share on other sites
Quote:
Original post by Sharlin
Actually, that won't work. It will skip the elements following the erased ones because the incrementing is done twice. Instead, you'll have to do this:

for (std::list<X>::iterator iter=x.begin(); iter!=x.end(); ) {
iter->DoStuff();
if (iter->IsDead()) iter=x.erase(iter);
else ++iter;
}



Ouch, you're right.
I'll be sitting in that corner over there.

Share this post


Link to post
Share on other sites

This topic is 4356 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