At 23:51 +0000 12/6/04, Neil Santos wrote:
constructor TBank.Create(amount : PCredit); beginNew(Self.AccountBalance); Self.AccountBalance := amount; end;
This pair of lines shows a deep lack of understanding of what a pointer is and how pointers work.
The first line allocates some memory for you and puts the address of the memory into the pointer variable Self.AccountBalance.
The second line copies the address from the pointer variable amount into the pointer variable Self.AccountBalance. Thus the address to the area of memory you allocated is now lost entirely, guaranteeing a memory leak. Memory leaks are not fatal (unless you leak enough), but they almost always indicate incorrect programming (the only real exception being memory you allocate once and never release and rely on the system to clean up for you when your application exits - and since you only allocate it once you have a finite amount of "leaked" memory so your program wont eventually die).
The next main problem with the routine is more conceptual - who owns the memory and who will be responsible for destroying it later? Any time you pass in a pointer and a copy of the pointer is kept, you are setting yourself up for problems unless you are very clear about the ownership of the pointer.
You program would be fine if you added a comment like:
(* The TBank.Create constructor's parameter is a pointer which must be a validly New'ed pointer and which becomes the property of TBank and which will be Dispose'd when the TBank object is Destroyed *)
and then you live up to that, both in TBank (where you would remove the New from TBank.Create) and outside of TBank (where you would never reference BankCredit again once you passed in to TBank.Create).
An alternate style would be:
(* The TBank.Create constructor's parameter is a pointer which must point to valid memory and must remain valid until after the TBank object is destoryed at which time it is your responsibility to dispose the pointer if required *)
and then live up to that, both in TBank (where the New and Dispose need to be removed) and your main program (which is correct except that the writeln should presumably be writeln(BankCredit^)).
[I've included the full original program below for completeness (sorry to any top-post-haters)]
Note that there is one very important and yet subtle difference between the two strategies. In the first strategy, the pointer must be allocated in a specific way such that TBank can know how to dispose of the pointer. In the second strategy, TBank only needs to get a valid pointer, it does not care how the pointer was allocated (so it could be an @BankCreditVariable, or could be created using a system routine like malloc or Mac's NewPtr or whatever). this is important because you can only Dispose a pointer that is created with New but you can get pointers in other ways (such as from a system library routine) that are still valid pointers and still point to memory, but cannot be disposed using Dispose.
As for how to avoid these sorts of errors, there are a bunch of methods:
Always initialize your variables. Always set pointers to nil once they are disposed. Never keep a pointer recorded in two different locations. And most importantly, heavily use assertions.
I highly recommend reading Writing Solid Code by Steve Maguire.
http://www.peter.com.au/books.html#WritingSolidCode
If you have not read this book or internalized the concept of using assertions and asking for every bug you find, "How could I have avoided introducing that bug?" then you should seriously consider reading or re-reading it.
Enjoy, Peter.
At 23:51 +0000 12/6/04, Neil Santos wrote:
program BankTest; varBankCredit : PCredit; BankSample : PBank;
beginNew(BankCredit); BankCredit^ := 1000;
New(BankSample, BankCredit); BankSample^.Destroy; writeln(BankSample^); Dispose(BankCredit); end.
TBank.Create has this:
constructor TBank.Create(amount : PCredit); beginNew(Self.AccountBalance); Self.AccountBalance := amount; end;
TBank.Destroy has this:
destructor TBank.Destroy; beginDispose(Self.AccountBalance); end;