Memory cleanup for TStringGrid Objects property?

[Remy Lebeau (TeamB)]

For this, I use TStringGrid and its Objects properties.

I would not suggest storing the actual objects in the grid directly. You
should store them in your own TList or other container, and then you can add
just pointers/indexes for your list into the Grid for easier access. This
way, you are completely in control of the memory of the objects, and when
and how that memory is cleaned up.

now I start to get unexspected Access Violations

Then you are likely using the memory incorrectly, or accessing it after it
has already been freed.

I seriously consider not bothering about memory cleanup

ALWAYS cleanup any memory you allocate yourself.

just because I cannot find the errors to make any sense

Have you tried using the debugger to help you? When an AV occurs, you have
a call stack leading up to the AV. You can back trace what code causes the
AV. There are also plenty of third-party tools that will help you
troubleshoot AVs as well, such as EurekaLog or MadExcept.

Maybe I'm overlooking the strange choice of the methods for
TStrings already doing the Objects cleanup for me??

No, it does not. You are responsible for the memory.

TStringList* SLP = new TStringList(); SLP->Add(FilePath);
StringGrid1->Objects[0][StringGrid1->RowCount-1]=SLP;
SLP = new TStringList(); SLP->LoadFromFile(FilePath+FileName);
StringGrid1->Objects[1][StringGrid1->RowCount-1]=SLP;
As I understand it, new Objects created in this way must

Yes, they do.

it seems there is no published method for deleting rows from the
stringgrid

TCustomGrid does have a DeleteRow() method, but it is protected. You can
use an accessor class to reach it, though:

class TStringGridAccess : public TStringGrid
{
public:
void __fastcall DeleteRow(int ARow)
{
TCustomGrid::DeleteRow(ARow);
}
};

((TStringGridAccess*)StringGrid1)->DeleteRow(StringGrid1->Row);

Take e.g. the case where three files have just been added.
Objects[0][1], Objects[0][2] and Objects[0][3] will point to newly created
StringList objects that I choose to call SL1, SL2 and SL3 here.
If then row 2 is removed, my FileDropExecute event will first delete the
StringList SL2.
After that, StringGrid1->Rows[2]=StringGrid1->Rows[3] and this is where
the access violation occurs.

Simply accessing the Rows[] property will not enough to cause an AV. Are
you sure your StringGrid pointer is actually valid at the time your code is
running?

Surely it cannot be that way that the Assign / operator = methods
themselves check for objects and delete them?

No, they do not.

But I cannot explain the Access Violation any other way.

You are either accessing wrong indexes or invalid pointers.

By the way, this doesn't explain at all why there is no
problem with deleting Objects[1][i]!

Is a non-NULL pointer being returned?

Try this code instead:

class TForm1 : public TForm
{
...
private:
TList *Files;
...
};


struct MyFileInfo
{
AnsiString FilePath;
TStringList *Contents;

MyFileInfo() : Contents(new TStringList) {}
~MyFileInfo() { delete Contents; }
};

__fastcall TForm1::TForm1(TComponent* Owner)
: TForm(Owner)
{
Files = new TList;
Defaults = new TStringList;
...
}

__fastcall TForm1::~TForm1()
{
for(int i = 0; i < Files->Count; ++i)
delete (MyFileInfo*) Files->Items[i];
delete Files;
delete Defaults;
}

void __fastcall TForm1::AssignProperties(int R, const AnsiString & P,
const AnsiString &N)
{
MyFileInfo *FileInfo = NULL;
try
{
FileInfo = new MyFileInfo;
FileInfo->FilePath = P;
FileInfo->Contents->LoadFromFile(P+N);
Files->Add(FileInfo);
}
catch(...)
{
delete FileInfo;
throw;
}

StringGrid1->Rows[R] = Defaults;
StringGrid1->Cells[0][R] = N;
StringGrid1->Objects[0][R] = (TObject*) FileInfo;
}

void __fastcall TForm1::FileOpen1Accept(TObject *Sender)
{
int p;
AnsiString Path,Name;
TStringList* SLP;

if( (StringGrid1->RowCount == 2) &&
!StringGrid1->Options.Contains(goEditing) )
{
StringGrid1->Options = StringGrid1->Options << goEditing;
ToolButton2->Enabled = true;
}
else
{
StringGrid1->RowCount = StringGrid1->RowCount+1;
}

for(int i = 0; i < FileOpen1->Dialog->Files->Count; ++i)
{
if( i > 0 )
StringGrid1->RowCount = StringGrid1->RowCount + 1;
ExtractPath(FileOpen1->Dialog->Files->Strings[i], Path, Name);
AssignProperties(StringGrid1->RowCount-1, Path, Name);
}
}

void __fastcall TForm1::DeleteObjects(int R)
{
//If more Objects are assigned per row we need only change this
routine
// delete StringGrid1->Objects[0][R];

MyFileInfo *FileInfo = (MyFileInfo*) StringGrid1->Objects[0][R];
StringGrid1->Objects[0][R] = NULL;

Files->Remove(FileInfo);
delete FileInfo;
}

void __fastcall TForm1::FileDropExecute(TObject *Sender)
{
// First, delete the Object properties of the row to be removed
DeleteObjects(StringGrid1->Row);

// Then copy row contents from rows below (unless RowCount==2)
for(int row = StringGrid1->Row+1; row < StringGrid1->Row; ++row)
{
for(int col = StringGrid1->ColCount-1; col >= 0; --col)
StringGrid1->Cells[col][row-1] =
StringGrid1->Cells[col][row];
StringGrid1->Objects[0][row-1] = StringGrid1->Objects[0][row];
}

// If RowCount==2, create a blank non-editable row
// Else, decrease number of rows, and move cursor up one row if
necessary
if( StringGrid1->RowCount == 2 )
{
for(int i = 0; i < StringGrid1->ColCount; ++i)
StringGrid1->Cells[i][1] = "";

ToolButton2->Enabled = false;
StringGrid1->Options = StringGrid1->Options >> goEditing;
}
else
{
if( StringGrid1->Row == StringGrid1->RowCount-1)
StringGrid1->Row = StringGrid1->Row-1;
StringGrid1->RowCount = StringGrid1->RowCount - 1;
}
}

void __fastcall TForm1::StringGrid1GetEditText(TObject *Sender, int
ACol, int ARow, AnsiString &Value)
{
if( (ARow > 0) && StringGrid1->Options.Contains(goEditing) )
{
if( ACol > 0 )
StatusBar1->SimpleText = "Default value: " +
Defaults->Strings[ACol];
else
{
MyFileInfo *FileInfo = (MyFileInfo*)
StringGrid1->Objects[0][ARow];
StatusBar1->SimpleText = "File path: " + FileInfo->FilePath;
}
}
}

void __fastcall TForm1::StringGrid1SetEditText(TObject *Sender, int
ACol, int ARow, const AnsiString Value)
{
if( (ARow > 0) && !StringGrid1->Options.Contains(goEditing) &&
(Value != "") )
StringGrid1->Cells[ACol][ARow] = "";
}

void __fastcall TForm1::StringGrid1DblClick(TObject *Sender)
{
TPoint P = StringGrid1->ScreenToClient(Mouse->CursorPos);
int C, R;
StringGrid1->MouseToCell(P.x, P.y, C, R);
if( (R == 0) && (C > 1) )
Defaults->Strings[C] = InputBox("Change default value", "New
default value (" + StringGrid1->Cells[C][0] + ")", Defaults->Strings[C]);
}


Gambit

[Erik Doekes]

Wow, that's a lot of work you put into answering me.
I think I'll start with a new fresh application!
Merci :)

TList and FileInfo struct don't sound like a bad idea! And with good control
of memory allocation...

From your code proposal, I take it you neither like accessing TCustomGrid's
DeleteRow() method nor using the operator = for TStrings *
TStringGrid::Rows - I guess then I'll follow your advice, and go for a
double loop copying Cells[col][row] instead...
I saw you made several other corrections along the code (how to handle Set
has worried me quite a lot e.g.), thanks again for your efforts!

/Erik

Post by Remy Lebeau (TeamB)

For this, I use TStringGrid and its Objects properties.

I would not suggest storing the actual objects in the grid directly. You
should store them in your own TList or other container, and then you can
add just pointers/indexes for your list into the Grid for easier access.
This way, you are completely in control of the memory of the objects, and
when and how that memory is cleaned up.

now I start to get unexspected Access Violations

Then you are likely using the memory incorrectly, or accessing it after it
has already been freed.

I seriously consider not bothering about memory cleanup

ALWAYS cleanup any memory you allocate yourself.

just because I cannot find the errors to make any sense

Have you tried using the debugger to help you? When an AV occurs, you
have a call stack leading up to the AV. You can back trace what code
causes the AV. There are also plenty of third-party tools that will help
you troubleshoot AVs as well, such as EurekaLog or MadExcept.

Maybe I'm overlooking the strange choice of the methods for
TStrings already doing the Objects cleanup for me??

No, it does not. You are responsible for the memory.

TStringList* SLP = new TStringList(); SLP->Add(FilePath);
StringGrid1->Objects[0][StringGrid1->RowCount-1]=SLP;
SLP = new TStringList(); SLP->LoadFromFile(FilePath+FileName);
StringGrid1->Objects[1][StringGrid1->RowCount-1]=SLP;
As I understand it, new Objects created in this way must

Yes, they do.

it seems there is no published method for deleting rows from the
stringgrid

TCustomGrid does have a DeleteRow() method, but it is protected. You can
class TStringGridAccess : public TStringGrid
{
void __fastcall DeleteRow(int ARow)
{
TCustomGrid::DeleteRow(ARow);
}
};
((TStringGridAccess*)StringGrid1)->DeleteRow(StringGrid1->Row);

Take e.g. the case where three files have just been added.
Objects[0][1], Objects[0][2] and Objects[0][3] will point to newly
created StringList objects that I choose to call SL1, SL2 and SL3 here.
If then row 2 is removed, my FileDropExecute event will first delete the
StringList SL2.
After that, StringGrid1->Rows[2]=StringGrid1->Rows[3] and this is where
the access violation occurs.

Simply accessing the Rows[] property will not enough to cause an AV. Are
you sure your StringGrid pointer is actually valid at the time your code
is running?

Surely it cannot be that way that the Assign / operator = methods
themselves check for objects and delete them?

No, they do not.

But I cannot explain the Access Violation any other way.

You are either accessing wrong indexes or invalid pointers.

By the way, this doesn't explain at all why there is no
problem with deleting Objects[1][i]!

Is a non-NULL pointer being returned?
class TForm1 : public TForm
{
...
TList *Files;
...
};
struct MyFileInfo
{
AnsiString FilePath;
TStringList *Contents;
MyFileInfo() : Contents(new TStringList) {}
~MyFileInfo() { delete Contents; }
};
__fastcall TForm1::TForm1(TComponent* Owner)
: TForm(Owner)
{
Files = new TList;
Defaults = new TStringList;
...
}
__fastcall TForm1::~TForm1()
{
for(int i = 0; i < Files->Count; ++i)
delete (MyFileInfo*) Files->Items[i];
delete Files;
delete Defaults;
}
void __fastcall TForm1::AssignProperties(int R, const AnsiString & P,
const AnsiString &N)
{
MyFileInfo *FileInfo = NULL;
try
{
FileInfo = new MyFileInfo;
FileInfo->FilePath = P;
FileInfo->Contents->LoadFromFile(P+N);
Files->Add(FileInfo);
}
catch(...)
{
delete FileInfo;
throw;
}
StringGrid1->Rows[R] = Defaults;
StringGrid1->Cells[0][R] = N;
StringGrid1->Objects[0][R] = (TObject*) FileInfo;
}
void __fastcall TForm1::FileOpen1Accept(TObject *Sender)
{
int p;
AnsiString Path,Name;
TStringList* SLP;
if( (StringGrid1->RowCount == 2) &&
!StringGrid1->Options.Contains(goEditing) )
{
StringGrid1->Options = StringGrid1->Options << goEditing;
ToolButton2->Enabled = true;
}
else
{
StringGrid1->RowCount = StringGrid1->RowCount+1;
}
for(int i = 0; i < FileOpen1->Dialog->Files->Count; ++i)
{
if( i > 0 )
StringGrid1->RowCount = StringGrid1->RowCount + 1;
ExtractPath(FileOpen1->Dialog->Files->Strings[i], Path, Name);
AssignProperties(StringGrid1->RowCount-1, Path, Name);
}
}
void __fastcall TForm1::DeleteObjects(int R)
{
//If more Objects are assigned per row we need only change this
routine
// delete StringGrid1->Objects[0][R];
MyFileInfo *FileInfo = (MyFileInfo*) StringGrid1->Objects[0][R];
StringGrid1->Objects[0][R] = NULL;
Files->Remove(FileInfo);
delete FileInfo;
}
void __fastcall TForm1::FileDropExecute(TObject *Sender)
{
// First, delete the Object properties of the row to be removed
DeleteObjects(StringGrid1->Row);
// Then copy row contents from rows below (unless RowCount==2)
for(int row = StringGrid1->Row+1; row < StringGrid1->Row; ++row)
{
for(int col = StringGrid1->ColCount-1; col >= 0; --col)
StringGrid1->Cells[col][row-1] =
StringGrid1->Cells[col][row];
StringGrid1->Objects[0][row-1] = StringGrid1->Objects[0][row];
}
// If RowCount==2, create a blank non-editable row
// Else, decrease number of rows, and move cursor up one row if
necessary
if( StringGrid1->RowCount == 2 )
{
for(int i = 0; i < StringGrid1->ColCount; ++i)
StringGrid1->Cells[i][1] = "";
ToolButton2->Enabled = false;
StringGrid1->Options = StringGrid1->Options >> goEditing;
}
else
{
if( StringGrid1->Row == StringGrid1->RowCount-1)
StringGrid1->Row = StringGrid1->Row-1;
StringGrid1->RowCount = StringGrid1->RowCount - 1;
}
}
void __fastcall TForm1::StringGrid1GetEditText(TObject *Sender, int
ACol, int ARow, AnsiString &Value)
{
if( (ARow > 0) && StringGrid1->Options.Contains(goEditing) )
{
if( ACol > 0 )
StatusBar1->SimpleText = "Default value: " +
Defaults->Strings[ACol];
else
{
MyFileInfo *FileInfo = (MyFileInfo*)
StringGrid1->Objects[0][ARow];
StatusBar1->SimpleText = "File path: " +
FileInfo->FilePath;
}
}
}
void __fastcall TForm1::StringGrid1SetEditText(TObject *Sender, int
ACol, int ARow, const AnsiString Value)
{
if( (ARow > 0) && !StringGrid1->Options.Contains(goEditing) &&
(Value != "") )
StringGrid1->Cells[ACol][ARow] = "";
}
void __fastcall TForm1::StringGrid1DblClick(TObject *Sender)
{
TPoint P = StringGrid1->ScreenToClient(Mouse->CursorPos);
int C, R;
StringGrid1->MouseToCell(P.x, P.y, C, R);
if( (R == 0) && (C > 1) )
Defaults->Strings[C] = InputBox("Change default value", "New
default value (" + StringGrid1->Cells[C][0] + ")", Defaults->Strings[C]);
}
Gambit

[Remy Lebeau (TeamB)]

Post by Erik Doekes
From your code proposal, I take it you neither like accessing
TCustomGrid's DeleteRow() method

I did not say that. I showed you how to access it if you want to.

Post by Erik Doekes
nor using the operator = for TStrings * TStringGrid::Rows

I never used it before, so I don't know whether to trust it or not.


Gambit

[Erik Doekes]

Post by Remy Lebeau (TeamB)

Post by Erik Doekes
From your code proposal, I take it you neither like accessing
TCustomGrid's DeleteRow() method

I did not say that. I showed you how to access it if you want to.

Post by Erik Doekes
nor using the operator = for TStrings * TStringGrid::Rows

I never used it before, so I don't know whether to trust it or not.

OK, that's a good answer ;)

Concerning DeleteRow(), I guess it does the same thing as the double
for.loop anyway, except I'd have to create a new class... so let's stick to
what we know and see ;)

Many thanks!

Erik

[Erik Doekes]

Now I've started with a new application, refilling in the code piece by
piece.

But with the following code, I experienced that in some cases, Files->Count
did not decrease and the destructor for FileInfo was not called either.

Post by Remy Lebeau (TeamB)
MyFileInfo *FileInfo = (MyFileInfo*) StringGrid1->Objects[0][R];
StringGrid1->Objects[0][R] = NULL;
Files->Remove(FileInfo);
delete FileInfo;

So I tried replacing it with:
MyFileInfo *FileInfo = (MyFileInfo*) Files->Items[R-1];
delete FileInfo;
Files->Delete(R-1);
which seems to be more reliable, although I haven't figured out why.

But this in turn makes me wonder why I should use the Objects property at
all in my case.
Because if my stringgrid only adds files at the end, and so does TList (?),
then surely I must be able to rely on the both index sets corresponding - so
that when I remove a row from the StringGrid, I can remove the object with
index one less (because of the StringGrid's header) from the TList?
In that case I guess I might just as well skip using the Objects property.

I also finally found a solution to the problem I commented in the code that
a cursor was still seen blinking although goEditing had been set to false.
This seems to be resolved by:
StringGrid1->EditorMode=false;
As a result of which, I can remove my OnSetEditText event (and save some CPU
effort?)

Erik

Post by Remy Lebeau (TeamB)
class TForm1 : public TForm
{
...
TList *Files;
...
};
struct MyFileInfo
{
AnsiString FilePath;
TStringList *Contents;
MyFileInfo() : Contents(new TStringList) {}
~MyFileInfo() { delete Contents; }
};
__fastcall TForm1::TForm1(TComponent* Owner)
: TForm(Owner)
{
Files = new TList;
Defaults = new TStringList;
...
}
__fastcall TForm1::~TForm1()
{
for(int i = 0; i < Files->Count; ++i)
delete (MyFileInfo*) Files->Items[i];
delete Files;
delete Defaults;
}
void __fastcall TForm1::AssignProperties(int R, const AnsiString & P,
const AnsiString &N)
{
MyFileInfo *FileInfo = NULL;
try
{
FileInfo = new MyFileInfo;
FileInfo->FilePath = P;
FileInfo->Contents->LoadFromFile(P+N);
Files->Add(FileInfo);
}
catch(...)
{
delete FileInfo;
throw;
}
StringGrid1->Rows[R] = Defaults;
StringGrid1->Cells[0][R] = N;
StringGrid1->Objects[0][R] = (TObject*) FileInfo;
}
void __fastcall TForm1::FileOpen1Accept(TObject *Sender)
{
int p;
AnsiString Path,Name;
TStringList* SLP;
if( (StringGrid1->RowCount == 2) &&
!StringGrid1->Options.Contains(goEditing) )
{
StringGrid1->Options = StringGrid1->Options << goEditing;
ToolButton2->Enabled = true;
}
else
{
StringGrid1->RowCount = StringGrid1->RowCount+1;
}
for(int i = 0; i < FileOpen1->Dialog->Files->Count; ++i)
{
if( i > 0 )
StringGrid1->RowCount = StringGrid1->RowCount + 1;
ExtractPath(FileOpen1->Dialog->Files->Strings[i], Path, Name);
AssignProperties(StringGrid1->RowCount-1, Path, Name);
}
}
void __fastcall TForm1::DeleteObjects(int R)
{
//If more Objects are assigned per row we need only change this
routine
// delete StringGrid1->Objects[0][R];
MyFileInfo *FileInfo = (MyFileInfo*) StringGrid1->Objects[0][R];
StringGrid1->Objects[0][R] = NULL;
Files->Remove(FileInfo);
delete FileInfo;
}
void __fastcall TForm1::FileDropExecute(TObject *Sender)
{
// First, delete the Object properties of the row to be removed
DeleteObjects(StringGrid1->Row);
// Then copy row contents from rows below (unless RowCount==2)
for(int row = StringGrid1->Row+1; row < StringGrid1->Row; ++row)
{
for(int col = StringGrid1->ColCount-1; col >= 0; --col)
StringGrid1->Cells[col][row-1] =
StringGrid1->Cells[col][row];
StringGrid1->Objects[0][row-1] = StringGrid1->Objects[0][row];
}
// If RowCount==2, create a blank non-editable row
// Else, decrease number of rows, and move cursor up one row if
necessary
if( StringGrid1->RowCount == 2 )
{
for(int i = 0; i < StringGrid1->ColCount; ++i)
StringGrid1->Cells[i][1] = "";
ToolButton2->Enabled = false;
StringGrid1->Options = StringGrid1->Options >> goEditing;
}
else
{
if( StringGrid1->Row == StringGrid1->RowCount-1)
StringGrid1->Row = StringGrid1->Row-1;
StringGrid1->RowCount = StringGrid1->RowCount - 1;
}
}
void __fastcall TForm1::StringGrid1GetEditText(TObject *Sender, int
ACol, int ARow, AnsiString &Value)
{
if( (ARow > 0) && StringGrid1->Options.Contains(goEditing) )
{
if( ACol > 0 )
StatusBar1->SimpleText = "Default value: " +
Defaults->Strings[ACol];
else
{
MyFileInfo *FileInfo = (MyFileInfo*)
StringGrid1->Objects[0][ARow];
StatusBar1->SimpleText = "File path: " +
FileInfo->FilePath;
}
}
}
void __fastcall TForm1::StringGrid1SetEditText(TObject *Sender, int
ACol, int ARow, const AnsiString Value)
{
if( (ARow > 0) && !StringGrid1->Options.Contains(goEditing) &&
(Value != "") )
StringGrid1->Cells[ACol][ARow] = "";
}
void __fastcall TForm1::StringGrid1DblClick(TObject *Sender)
{
TPoint P = StringGrid1->ScreenToClient(Mouse->CursorPos);
int C, R;
StringGrid1->MouseToCell(P.x, P.y, C, R);
if( (R == 0) && (C > 1) )
Defaults->Strings[C] = InputBox("Change default value", "New
default value (" + StringGrid1->Cells[C][0] + ")", Defaults->Strings[C]);
}
Gambit

[Remy Lebeau (TeamB)]

Post by Erik Doekes
But with the following code, I experienced that in some cases,
Files->Count did not decrease, and the destructor for FileInfo
was not called either.

The only way that can happen is if Remove() does not find the provided
pointer in the list, thus does not have anything to remove. For instance,
make sure the pointer retreived from the Objects[][] property is not NULL.
<snip>

Post by Erik Doekes
which seems to be more reliable, although I haven't figured out why.

If you are only removing 1 item, and you already know its exact index, then
yes, you could use Delete() instead of Remove().

Post by Erik Doekes
But this in turn makes me wonder why I should use the Objects
property at all in my case.

You don't really need to anymore. The more you can separate your data logic
from your UI logic, the better your code will be in general.

Post by Erik Doekes
Because if my stringgrid only adds files at the end, and so does
TList (?), then surely I must be able to rely on the both index
sets corresponding

Yes. Even if you do random insertions, rather than at the end only, you can
still keep the two in sync at all times.

Post by Erik Doekes
so that when I remove a row from the StringGrid, I can remove the
object with index one less (because of the StringGrid's header) from
the TList?

Yes.


Gambit