FreeBASIC Good Practices Discussion

For other topics related to the FreeBASIC project or its community.
Pritchard
Posts: 5477
Joined: Sep 12, 2005 20:06
Location: Ohio, USA

FreeBASIC Good Practices Discussion

Postby Pritchard » Feb 27, 2010 1:47

The mistakes most commonly made are rooted in bad practice, so I thought it'd be fun if we could share tips, dos and do nots of programming. Pointers are probably most vital:

1. Type Safety with Pointers

Avoid using a generic pointer type (any/void pointers) if the code isn't designed specifically for it. Example:

Code: Select all

#include "fbgfx.bi"
sub putImage(img as any ptr) '' should be:  img as fb.Image ptr
    dim as fb.Image ptr fbImg = cast(fb.Image ptr, img)  '' not safe
    put (0, 0), fbImg, pset
end sub

screenres 640, 480, 32
dim as fb.Image ptr myImage = imageCreate(640, 480)
putImage(myImage)
sleep
imageDestroy(myImage) '' free up memory space
img as any ptr in putImage should be img as fb.Image ptr. Now, no type-casting is necessary, and you're ensuring that putImage will accept ONLY fb.Image ptrs. This is one way to enfore type-safety and to prevent those annoying "segmentation fault" crash reports.

Also note that we imageDestroy(myImage). Resource management can be confusing, but just know that any memory you allocate (or "reserve" for use by your program) should be deallocated (or "freed" for use by other programs). Always check the manual when creating resources to see how to properly manage them.

2. Null your pointers:

Even though the Operating System will free memory once your program has terminated, while it is still running resources continue to occupy space. Just as dangerous are resources which are allocated, freed, but not zero'd out (made into a null pointer):

Code: Select all

dim as integer ptr integerList = new integer[1000]
for i as integer = 0 to 999: integerList[i] = rnd()*1000: next
delete[] integerList
integerList = 0 '' necessary if delete[] doesn't automatically 0 integerList

if( integerList <> 0 ) then
    '' if integerList wasn't set to 0, but the memory was freed, this program ideally crashes immediately, but often doesn't.
    for i as integer = 0 to 999: print integerList[i]: next
end if

Pointers are simply integers which "point" to a location in memory (usually RAM). Allocated pointers never equal 0, so we use 0 to indicate "null", or empty pointers. Forgetting to 0 out pointers can lead to thinking memory has been allocated when it hasn't.

Unfortunately, our ideal case of programs crashing immediately upon faulty access does not always occur. Misuse of pointers can cause unpredictable, difficult to trace bugs in code, when the OS doesn't even know you're accessing data you shouldn't be, giving you no indication of faulty memory access.
Last edited by Pritchard on Feb 28, 2010 15:47, edited 1 time in total.
agamemnus
Posts: 1842
Joined: Jun 02, 2005 4:48

Postby agamemnus » Feb 27, 2010 2:18

O wow, I never thought of 2. But I generally don't reuse pointers like that, anyway..
Pritchard
Posts: 5477
Joined: Sep 12, 2005 20:06
Location: Ohio, USA

Postby Pritchard » Feb 27, 2010 3:39

agamemnus wrote:O wow, I never thought of 2. But I generally don't reuse pointers like that, anyway..
It's not necessarily about "reuse". This kind of pointer usage is very common and inherent to most dynamic structures. Linked lists, for example:

Code: Select all

'' simple linked list example
type NodeFwd as Node '' Just to be safe.  I don't have fbc so I can't test if I can point to Node within itself...
type Node
   as integer dat '' data in node - just to show how this works
   as NodeFwd ptr prv '' previous node in list
   as NodeFwd ptr nxt '' next node in list (kind of like arrays)
end type

'' start our linked list
dim as Node ptr cNode = callocate(sizeof(Node))

'' add a node ahead of us
cNode->nxt = callocate(sizeof(Node))
'' add a node behind us
cNode->prv = callocate(sizeof(Node))

'' our list currently looks like this:  a<-->b<-->c
'' we're going to do this:  a<-->c, so nothing points to b.
cNode->nxt->prv = cNode->prv '' node ahead (c) now points to back (a)
cNode->prv->nxt = cNode->nxt '' node behind (a) now points to front (c)

'' Now make cNode point to a, and remove our island node (b) from existence
dim as Node ptr temp = cNode
cNode = cNode->prv
deallocate(temp)
A linked list will end when either the prv or nxt = 0. It we erased prv or nxt, but forgot to reassign or nullify them, we'd think they were still a part of the linked list and likely run into serious trouble.

Even a simple dynamic array has to keep these things in mind. Callocate can actually return null if memory space is insufficient!
1000101
Posts: 2556
Joined: Jun 13, 2005 23:14
Location: SK, Canada

Re: FreeBASIC Good Practices Discussion

Postby 1000101 » Feb 27, 2010 15:50

Pritchard wrote:Even though the Operating System will free memory once your program has terminated


That generalization is untrue and leads to faultly logic. A lot of OS's do not free resources after a program has terminated which is why you need to do it explicitly. There is no garuntee what OS you will run under or what the OS does or does not do for you as far as resources are concerned.
ShawnLG
Posts: 80
Joined: Dec 25, 2008 20:21

Postby ShawnLG » Feb 27, 2010 17:26

When allocating memory for dynamic data structures. They should always be put in a OOP style data type, such as using constructors and destructors. In a type defined data type your constructor will allocate and setup your data structure. The destructure will deallocate and clean up your data structure when your program quits. This keeps everything encapsulated so you do not need to worry about the inner workings while concentrating on the 'bigger picture' of your program.

Below is an example of my bitmap loading routine. It is encapsulated in a defined type called BMPimage.

When using this routine I would DIM a BMPimage object.
Example:

DIM AS BMPimage a

Now 'a' is a defined type of BMPimage. Memory for BMP data has not been allocated yet because we did not load a BMP file. When this happends, the defult constructor() is called and NULLs all pointers and variables inside the BMPimage defined type. This is nessesary so that we know that 'a' has no bitmap image. When we load a BMP while we DIM, it would look like this:

DIM AS BMPimage a = BMPimage("bmpfile.bmp")

The line above is the save as previous example, but it will call the constructor(filename as string) instead of the defult one.

When the program or the sub procedure that the BMPimage was defined in exits. The destructor() is called. This is where you deallocate your memory that your object has used.



Code: Select all

'BMPloader.bas by Shawn Grieb (c) 2010
'This loader only works for uncompressed 8 or 24 bit bitmaps with the V3 headerinfo.
type BMPFileHeader field=1
    dim as ushort Magic 'Should be "BM"
    dim as uinteger BMPsize ' Size of BMP file
    dim as ushort Res1 ' Reserved
    dim as ushort Res2
    dim as uinteger OffsetData ' starting addr. of BMP data
    dim as uinteger BMPheadersize 'ususally 40 bytes
end type

'BMPheader size
'12 OS/2 V1 BITMAPCOREHEADER OS/2 and also all Windows versions since Windows 3.0
'64 OS/2 V2 BITMAPCOREHEADER2 
'40 Windows V3 BITMAPINFOHEADER all Windows versions since Windows 3.0
'108 Windows V4 BITMAPV4HEADER all Windows versions since Windows 95/NT4
'124 Windows V5 BITMAPV5HEADER Windows 98/2000 and newer

type BMPInfoHeader field=1
    dim as integer ImageWidth
    dim as integer ImageHeight
    dim as ushort NumPlains ' must be set to 1
    dim as ushort ImageBitDepth ' the number of bits per pixel, which is the color depth of the image. Typical values are 1, 4, 8, 16, 24 and 32
    dim as uinteger Compression ' 0 for none
    dim as uinteger ImageSize ' the size of the raw bitmap data
    dim as integer WidthPPM ' pixel per meter
    dim as integer HightPPM ' pixel per meter
    dim as uinteger NumOfColors
    dim as uinteger NumOfImpColors
end type

type BMPimage
    declare constructor ()
    declare constructor (filename as string )
    declare destructor ()
    declare sub InitBMP()
    declare function LoadBMP(filename as string) as integer
    declare function LoadBMPnormals(filename as string) as integer
    declare sub FreeBMP()
    declare function BMPptr() as any ptr
    declare function Xsize() as integer
    declare function Ysize() as integer
    dim as uinteger ImageShiftWidth
    dim as uinteger ImageBitMask
    dim as ubyte ptr pImageData
    dim as uinteger ptr pPalette
    private:
    declare sub ConvertToNormals()
    dim as integer ImageWidth
    dim as integer ImageHeight
    dim as uinteger BitDepth
end type

constructor BMPimage ()
    this.InitBMP()
end constructor

constructor BMPimage (filename as string)
    this.LoadBMP(filename)
end constructor

destructor BMPimage ()
    this.FreeBMP()
end destructor

function BMPimage.BMPptr() as any ptr
    return this.pImageData
end function

function BMPimage.Xsize() as integer
    return this.ImageWidth
end function

function BMPimage.Ysize() as integer
    return this.ImageHeight
end function

sub BMPimage.InitBMP()
    this.ImageWidth = 0
    this.ImageShiftWidth = 0
    this.ImageBitMask = 0
    this.ImageHeight = 0
    this.BitDepth = 0
    this.pImageData = 0
    this.pPalette = 0
end sub

sub BMPimage.ConvertToNormals()
    dim as integer c,r,g,b
    select case this.BitDepth
    case 8
        for i as integer = 0 to 255
            c = this.pPalette[i]
            r = (c shr 16) and &h000000ff
            g = (c shr 8) and &h000000ff
            b = c and &h000000ff
            r = r - 128
            g = g - 128
            b = b - 128
            c = (r shl 16) or (g shl 8) or b
            this.pPalette[i] = c
        next i
    case 24
        for i as integer = 0 to this.ImageWidth * this.ImageHeight - 1
            c = this.pImageData[i]
            r = (c shr 16) and &h000000ff
            g = (c shr 8) and &h000000ff
            b = c and &h000000ff
            r = r - 128
            g = g - 128
            b = b - 128
            c = (r shl 16) or (g shl 8) or b
            this.pImageData[i] = c
        next i
    end select
end sub

function BMPimage.LoadBMP(filename as string) as integer
    dim as BMPFileHeader fileheader
    dim as BMPInfoHeader infoheader
    dim as ubyte onebyte,rbyte,gbyte,bbyte
    dim as integer fileoffset, x, y, padded
    dim as integer filehandle, loaderror = 0
    dim as any ptr bufferoffset
    filehandle = freefile
    open filename for binary as #filehandle
    get #filehandle,,fileheader ' get fileheader
    if fileheader.magic = 19778 then
        fileoffset = fileheader.Offsetdata
        get #filehandle,, infoheader ' get infoheader
        this.freeBMP() 'free the last image if there was one
        this.ImageWidth = infoheader.ImageWidth
        this.ImageHeight = infoheader.ImageHeight
        this.ImageShiftWidth = log(abs(this.ImageWidth)) / log(2)
        this.ImageBitMask = not (&hffffffff shl this.ImageShiftWidth)
        this.BitDepth = infoheader.ImageBitDepth
        select case infoheader.ImageBitDepth
        case 8
            this.pImageData = allocate(infoheader.ImageWidth*infoheader.ImageHeight+256*4)
            bufferoffset = this.pImageData + infoheader.ImageWidth*infoheader.ImageHeight
            this.pPalette = bufferoffset
            for i as integer = 0 to 255
                get #filehandle,,this.pPalette[i]
            next i
            seek #filehandle,fileoffset+1
            padded = infoheader.ImageWidth mod 4
            for y = -(infoheader.ImageHeight-1) to 0
                for x = 0 to infoheader.ImageWidth-1
                    get #filehandle,, onebyte
                    this.pImageData[x+(-y)*infoheader.ImageWidth] = onebyte
                next x
                if padded then
                    for x = padded to 3
                        get #filehandle,, onebyte
                    next x
                end if
            next y
        case 24
            seek #filehandle,fileoffset+1
            this.pImageData = allocate(infoheader.ImageWidth*infoheader.ImageHeight*4)
            padded = infoheader.ImageWidth mod 4
            for y = -(infoheader.ImageHeight-1) to 0
                for x = 0 to infoheader.ImageWidth*4-1 step 4
                    get #filehandle,, bbyte
                    get #filehandle,, gbyte
                    get #filehandle,, rbyte
                    this.pImageData[(-y)*(infoheader.ImageWidth*4)+x] = bbyte
                    this.pImageData[(-y)*(infoheader.ImageWidth*4)+x+1] = gbyte
                    this.pImageData[(-y)*(infoheader.ImageWidth*4)+x+2] = rbyte
                    this.pImageData[(-y)*(infoheader.ImageWidth*4)+x+3] = 0
                next x
                if padded then
                    for x = padded to 3
                        get #filehandle,, onebyte
                    next x
                end if
            next y
        case else
            print "Unsupported bit depth: ";filename
            sleep
            loaderror = -1
        end select
    else 'Bitmap load failure
        print "Not a BMP file: ";filename
        sleep
        loaderror = -1
    end if
    close #filehandle
    return loaderror
end function

function BMPimage.LoadBMPnormals(filename as string) as integer
    dim as integer loaderror = this.LoadBMP(filename)
    if loaderror = 0 then
        this.ConvertToNormals()
    end if
    return loaderror
end function

sub BMPimage.FreeBMP()
    if this.pImageData <> 0 then ' Check if pImageData was allready allocated
        deallocate(this.pImageData)
        this.ImageWidth = 0
        this.ImageShiftWidth = 0
        this.ImageHeight = 0
        this.BitDepth = 0
        this.pImageData = 0
        this.pPalette = 0
    end if
end sub
marcov
Posts: 2623
Joined: Jun 16, 2005 9:45
Location: Eindhoven, NL
Contact:

Re: FreeBASIC Good Practices Discussion

Postby marcov » Feb 27, 2010 18:46

1000101 wrote:
Pritchard wrote:Even though the Operating System will free memory once your program has terminated


That generalization is untrue and leads to faultly logic. A lot of OS's do not free resources after a program has terminated which is why you need to do it explicitly. There is no garuntee what OS you will run under or what the OS does or does not do for you as far as resources are concerned.


Afaik only very old OSes and embedded OSes don't deallocate memory on shutdown.

Very old OSes are, euh, old, and embedded OSes applications typically don't terminate anyway.

But maybe I'm wrong, any good examples?

(btw, I agree though that you shouldn't rely on this though, but my reasons are different. It makes it harder to find true memory leaks because of all of the "noise" of undisposed ptrs)
D.J.Peters
Posts: 7428
Joined: May 28, 2005 3:28

Postby D.J.Peters » Feb 27, 2010 19:07

Allocated application (virtual) ram space are handled by OS (Linux and Windows)
but OS resources like bitmaps, GC's, DC's, Brushes, Pens ...
must be deallocated by the application (same on Linux)

Joshy
Pritchard
Posts: 5477
Joined: Sep 12, 2005 20:06
Location: Ohio, USA

Postby Pritchard » Feb 27, 2010 23:54

Summary of latest posts:

1) Make use of Ctors/Dtors to automatically manage resources, from ShawnLG.

2) Be aware that some resources (such as file hooks) must be 'manually' freed, from D.J. Peters.

3) @ShawnLG - Use private to hide data, otherwise your get() functions just add bloat. You over-encapsulate. You're replacing 1 line of code with 1 line of code. A slower line, at that.

Efficiency(%) = (LOCold/LOCnew) * 100%

Code: Select all

type MyType
    as integer foo '' shared by both - if we included this, it would nearly balance out in large number, but for a small example it skews our results
    declare function getFoo() as integer '' LOCnew += 1
    declare sub setFoo(newFoo as integer) '' LOCnew += 1
end type

function MyType.getFoo() as integer '' LOCnew += 1
    return foo  '' LOCnew += 1
end function '' LOCnew += 1

sub MyType.setFoo(newFoo as integer) '' LOCnew += 1
    foo = newFoo '' LOCnew += 1
end fun '' LOCnew += 1

dim as MyType temp
temp.foo = 10 '' LOCold += 1
print temp.foo '' LOCold += 1
sleep

temp.setFoo(10) '' LOCnew += 1
print temp.getFoo() '' LOCnew += 1
sleep

'' totals:  LOCold = 2, LOCnew = 10
'' LOCold/LOCnew * 100% = 20% efficiency.  Productivity has actually been reduced 80%.
We can factor in efficiency over time by making LOCold fluctuate and keeping LOCnew static (which would give us maintenence costs), but since a simple BMP class isn't expected to change internally, there are few (or no) maintenence costs. You remain 80% less efficient.
marcov
Posts: 2623
Joined: Jun 16, 2005 9:45
Location: Eindhoven, NL
Contact:

Postby marcov » Feb 28, 2010 17:21

D.J.Peters wrote:Allocated application (virtual) ram space are handled by OS (Linux and Windows)
but OS resources like bitmaps, GC's, DC's, Brushes, Pens ...
must be deallocated by the application (same on Linux)


Afaik only on w9x (which I consider an old OS), and not on NT. (where resources are per process and not a global pool)

I never heard of permanent resource leaks on *nix either, unless the resources were related to very specific circumstances like interprocess communication.

While I'd be interested to see references, that is obviously not what I was talking about, which was mainly memory.
D.J.Peters
Posts: 7428
Joined: May 28, 2005 3:28

Postby D.J.Peters » Feb 28, 2010 17:38

hello marcov
the most windows resources has internal an reference counter
if you don't use DeleteObject(), ReleaseDC() ...
the reference counter will never be zero and memory leaks are the result.

Joshy
1000101
Posts: 2556
Joined: Jun 13, 2005 23:14
Location: SK, Canada

Postby 1000101 » Feb 28, 2010 19:47

Whether or not the OS handles releasing resources your program requested is pretty irrelevant. I was just using memory and OS as a simple example. This goes beyond system resources, library resources, etc, etc. The idea is that you are responsible for your resources, nobody else. If you allocate the resource you should free resource. It's got to do with the main thread of "good programming practices" (which are not specific to any language).
ShawnLG
Posts: 80
Joined: Dec 25, 2008 20:21

Postby ShawnLG » Feb 28, 2010 20:14

3) @ShawnLG - Use private to hide data, otherwise your get() functions just add bloat.


Some data items are deliberately left public because it is much faster to access them directly then through a function. I know this is not good practice but there is a line between program readability and speed.

You over-encapsulate. You're replacing 1 line of code with 1 line of code. A slower line, at that.


I assume you mean somthing like this.

Code: Select all

constructor BMPimage (filename as string)
    this.LoadBMP(filename)
end constructor


I do this because sometimes I would need to manually load a bitmap. This happends when I create a pointer of type BMPimage. Such as:

DIM AS BMPimage PTR a

At this point, I can't load a bitmap because 'a' is only a pointer. I would need to allocate memory for it.

a = ALLOCATE(SIZEOF(BMPimage))

Now we can load a bitmap, but the constructors and distructors are not called. So I use LoadBMP() and FreeBMP() to manage it manually. In Bowser's Story the bitmap images are loaded dynamicly using pointers. So I have another class structure on top of that such as the Animation Class which manages the allocation and deallocation of the bitmaps.
Pritchard
Posts: 5477
Joined: Sep 12, 2005 20:06
Location: Ohio, USA

Postby Pritchard » Feb 28, 2010 21:49

Code: Select all

Function BMPimage.BMPptr() As Any Ptr
    Return this.pImageData
End Function

Function BMPimage.Xsize() As Integer
    Return this.ImageWidth
End Function

Function BMPimage.Ysize() As Integer
    Return this.ImageHeight
End Function
I was referring to these lines.
Cherry
Posts: 351
Joined: Oct 23, 2007 12:06
Location: Austria
Contact:

Postby Cherry » Jun 10, 2010 12:52

ShawnLG wrote:a = ALLOCATE(SIZEOF(BMPimage))

Now we can load a bitmap, but the constructors and distructors are not called.


Why ALLOCATE? I think, a = New BMPimage(.......) would do the trick (and also call the C/Dtors).

EDIT: Oh, sorry for necroposting. Didn't see the date...
segin
Posts: 126
Joined: Dec 27, 2005 5:22
Contact:

Postby segin » Jun 10, 2010 21:56

I'd think it'd be great if this kind of information gets rolled up into a wiki article. At least then it'll get rolled up into the chm, which is what a lot of n00bs use it seems.

Return to “Community Discussion”

Who is online

Users browsing this forum: Bing [Bot], Foneo and 2 guests