Author Topic: A quick question  (Read 2530 times)

0 Members and 1 Guest are viewing this topic.

Code: [Select]
void Unregister(IEngineModule* module)
{
  for (std::vector<IEngineModule*>::iterator i = engineModules_.begin(); i != engineModules_.end(); ++i) {
if (*i == module) {
engineModules_.erase(i);
break;
}
  }
}
Can anyone of you imagine a situation where this code could throw?
Would be important because it's a function that gets called in a destruktor.
I don't know if I should just wrap it in a try/catch block, let remain it like this because it won't throw anyway, wrap the call in a try/catch block or look for another solution.

[edit]
Seems like "erase" can't throw, so that would leave "*i" as a possible problem..?
« Last Edit: November 18, 2009, 08:15:38 am by Uchuujinsan »

 

Offline blackhole

  • Still not over the rainbow
  • 29
  • Destiny can suck it
    • Black Sphere Studios
Is there any particular reason why this code is using the vector iterator in the first place? Seems like a better idea to just do:

Code: [Select]
for(int i = 0; i < engineModules_.size(); ++i)
{
if(engineModules_[i] == module)
{
engineModules_.erase(engineModules_.begin()+i)
break;
}
}

In addition, whenever code is using a for loop to loop through an array to find and delete something, there's a better way to do it.

Also, this code doesn't check to see if the module that's sent to the function is a NULL pointer, which could blow something up.

 
I'm always using iterators if I'm iterating through a STL container, because it's typed it will make it harder to do stupid things with it.
And
Code: [Select]
engineModules_.begin()+i is using the vector iterator as well  - begin() returns an iterator :/

Quote
In addition, whenever code is using a for loop to loop through an array to find and delete something, there's a better way to do it.
Could you further explain this?

Quote
Also, this code doesn't check to see if the module that's sent to the function is a NULL pointer, which could blow something up.
The pointer here is never used to access something in that function, so it shouldn't be able to make any problems.

 

Offline blackhole

  • Still not over the rainbow
  • 29
  • Destiny can suck it
    • Black Sphere Studios
(engineModules_.begin()+i) is only using an iterator because there's not other way of deleting something.

Quote
The pointer here is never used to access something in that function, so it shouldn't be able to make any problems.

But if it IS a null pointer then suddenly you could have a situation where (engineModules_[ i] == module) evaluates to TRUE when both are NULL and then everything goes to hell.

Quote
Could you further explain this?

Well the problem is simply that goign through a for loop to find something is always the least efficient way of doing it. It depends on how its used elsewhere and how often the remove funciton is being called. Usually when you are iterating through an array you need to consider whether or not it would be better as a linked list of some sort.

 
(engineModules_.begin()+i) is only using an iterator because there's not other way of deleting something.
Well, then I can use iterators from the beginning anyway, makes it easier to switch to another container, because a list for example doesn't support
Code: [Select]
engineModules_.begin()+i
Quote
The pointer here is never used to access something in that function, so it shouldn't be able to make any problems.

But if it IS a null pointer then suddenly you could have a situation where (engineModules_[ i] == module) evaluates to TRUE when both are NULL and then everything goes to hell.
Well, if both are null at a completely valid position i, than that value is supposed to be removed at that position i, and it will be removed.

Quote
Could you further explain this?

Well the problem is simply that goign through a for loop to find something is always the least efficient way of doing it. It depends on how its used elsewhere and how often the remove funciton is being called. Usually when you are iterating through an array you need to consider whether or not it would be better as a linked list of some sort.
[/quote]
Ah, you don't mind the iterating itself, but the possible erase call that is inefficient with a vector (iterating through a list isn't faster).
Well, that's right, I could change it to a list, it's been a  vector before that erase call got added.

 

Offline blackhole

  • Still not over the rainbow
  • 29
  • Destiny can suck it
    • Black Sphere Studios
Quote
(iterating through a list isn't faster).

Yes it is. Regardless of that, however, removal from a list is a O(1) operation if its doubly linked.

Quote
Well, then I can use iterators from the beginning anyway, makes it easier to switch to another container, because a list for example doesn't support

If I were implementing a list I'd just do it manually, its faster that way and its not very hard.

Quote
Well, if both are null at a completely valid position i, than that value is supposed to be removed at that position i, and it will be removed.

Well, no, you could hit a fake NULL value and have that removed instead, but yes it would never throw an actual error.

...

Going back to the original problem, does Erase call an object's destructor? Your error might be in there instead.

 
I don't have an error, I want to prevent that while an exception gets thrown a destructor call will be invoked that might throw a second exception while the first one is still in effect, aborting the programm and preventing me from handling the error. And that's why I want to ensure that this code doesn't throw and was asking if I should just use try/catch (maybe if I might change that code later, so I can easily introduce a throw), or leave at that because it can't throw anyway.
I couldn't see a possibility how it could throw, and was curious if I was right because I wasn't that sure.

 

Offline Spicious

  • Master Chief John-158
  • 210
Pointers don't have destructors. *i should work as long as the iterator is valid. Trying to treat a container generically is usually a bad idea.

You probably should be using something like remove_if if you want to do it properly though.

 
... Please read from the beginning.

I resolved the question now, but I post an example anyway:

Code: [Select]
void Unregister()
{
std::vector<int> tmp;
tmp.push_back(3);
}

void Bar()
{
IEngineModule IEM;
throw "blubb";
}

class IEngineModule {
IEngineModule()
{
}
~IEngineModule()
{
Unregister();
}
};

int main()
{
try {
Bar();
}
catch(...)
{
std::cout<<"Error"; //Under certain circumstances this line won't be reached
}
std::cout<<"Success"; //and neither will be this one(!)
}
Which is a bad thing to happen.
Further read here:
http://www.parashift.com/c++-faq-lite/exceptions.html#faq-17.3

 

Offline Spicious

  • Master Chief John-158
  • 210
I did read from the beginning, but now this thread reminds me of this wtf.

 
Well, I don't have to pull the plug, a simple "out_of_memory" exception by tmp.push_back(3) will result in terminate() being called :>

 

Offline Spicious

  • Master Chief John-158
  • 210
Given that sensible stl implementations would allocate some space initially you'd probably get your exception in the constructor instead.

On a more general level, what do you really expect to do if you run out of memory?

 
We are already at a hypothetical level, because that code is save, so..

Most reasonable would be to free memory, save data, and end the programm (with an error message). But the main reason is, that I - in this case - don't care about the out of memory itself, if I would I'd have to have a lot of more try/catch in my code, but about the possibility to debug some other part of the code that threw an (unknown) exception I never get to see because of terminate().


If
Code: [Select]
std::vector<int> tmp; already throws, that wouldn't change the problem at all.

I simply have to ensure that an exception can never leave the destructor, and that was what I was trying to check her..

 

Offline Spicious

  • Master Chief John-158
  • 210
How could you decide what memory to free? Would the files to be saved need to be opened requiring more memory?

 
That depends on the application, but normally you can always nuke the graphics routine and unload everything there - if you are going to crash anyway, that shouldn't be a problem. In fact you can design the programm in such a way, that if you get an out of memory exception, the stack unwind will free everything that's not crucial data by iteself. Then catch it and use the freed memory to save that crucial data.
That works as long as an destructor during stack unwind won't throw :>