Sun 11 Nov 2012 08:40:07 AM UTC, comment #11:
The first delete should not cause a problem. This probably requires some digging to find out exactly how memory is deleted, more than I care to know about Qt at the moment. But one of two things could happen:
1) Deleting _current_directory_combo_box destroys the object then there is an error when the container class attempts to delete the object it had taken ownership of, but we can't see the error for some reason.
2) Deleting _current_directory_combo_box destroys the object and a base class of the widget notifies its parent that it is being destroyed and the container class knows to not attempt a delete.
> Is the test for 0 before deleting necessary?
As for testing whether a pointer is zero before testing, my first exposure to C++ was when it just hit the scene with Straustrup's book. I can't recall that rule. Maybe it's always been the case, maybe it was added after the initial definition. I will investigate.
Generally speaking, it might be good to test if there is more to the conditional than just deleting the pointer. For example:
if (lexer)
{
delete lexer;
_edit_area->setLexer(0);
}
also does something if the lexer was valid and is no longer valid. But I'll look this over.
> Should we set the pointers to 0 afterwards in order to be sure a second delete would be harmless?
Generally speaking, that is good practice but the cases I've added are in the destructor. The destructor being the last thing called, those member variables are on the verge of going out of scope so won't be useful any more. (An optimizing compiler might know enough to remove extraneous assignments like that.) I've removed such a line (but haven't made the patch yet).
> This would allow to delete _current_directory_combo_box (just to be sure)
I'd say 'no'. I.e., don't delete something that we know will be deleted elsewhere. If there is some statistical uncertainty about things, then maybe. But better to understand how things work and not have extraneous code. What you are suggesting doesn't work, I believe. The reason is that once the object is handed off to the container class, it has no knowledge of _current_directory_combo_box; that container isn't going to modify our member variable copy of it (but maybe its own record). We keep _current_directory_combo_box around so that we can access the object but it is technically no longer ours, as I understand it. (And maybe the better way to do this is to set up a signal/slot connection before handing the object off...because I do know that if objects on either end of a signal/slot are deleted, the signal connection is removed.
I was going to update the patch with the scheme we discussed on the list, but I've been thinking a couple things: 1) lack of easy way to get file information, 2) buffered vs. unbuffered file access, and maybe we need to be careful on what we choose because often it will be the case that an editor file is saved and then immediately run in the command line thread. We might want to be more cognizant of what it is we're doing exactly. I'm worried that saving a file using buffered i/o might not be complete and readable by the octave command line thread.
|
Sat 10 Nov 2012 09:35:39 AM UTC, comment #8:
What about _current_directory_combo_box? Shouldn't this be deleted, too?
On my system, I am am seeing the following error message on exit (with or without the patch)
Also a hint for some resource leaks, I guess.
|