Need someone to look at my code and suggest improvements

[axtens]

trouble is, it's about 2500 lines. I've got no one else around to discuss it with (I get raised eyebrows every time I confess to writing in assembler). Please contact me off-list if you have time: bruce AT strapper DOT net.

Thanks in advance,
Bruce.

[sluggy]

What exactly does your code do? If you post snippets in this forum for everyone to see, you will get a reasonable consensus and good conversation about it  :U

[axtens]

Okay, fair enough.

The DLL is known internally as BOSE (Bruce's Own String Engine). At present it's a standard DLL but may get turned into a COM DLL sometime soon. Eventually BOSE will be released under the LGPL.

BOSE does string handling. The lead developer wanted something that would work faster than VB6 and suggested assembler. So the faster all this is, the better.

BOSE provides 17 "slots" into which bytes may be stored. There are functions for slot management and data management. There's also an error reporting subsystem.

The STRUCT describing a slot is as follows:

Code Select Expand

Slot STRUCT
    instantiated dd 0
    slotbase     dd 0
    slotlength   dd 0
    textbase     dd 0
    textlength   dd 0
    occupied     dd 0
Slot ends

Storage for the slot array is allocated as:

Code Select Expand

    Slots    Slot (SLOT_COUNT + 1) dup (<0,0,0,0,0,0>)

When storage within a slot is allocated, the address of it is stored in slotbase and the size in slotlength. Text is stored near the centre of the allocation, so that prepending and appending can be done without too much shifting around. The address of the start of text is stored in textbase, the length in textlength. Slots must be instantiated to be used. Slots can be unoccupied (with data) thus the occupied element.

The functions that are exported are as follows:

Code Select Expand

;Exported functions
SEAppend                            PROTO    :DWORD
SEAppendFrom                        PROTO    :DWORD
SECompare                           PROTO    :DWORD, :DWORD
SECopySlot                          PROTO    :DWORD, :DWORD
SECountStack                        PROTO
SEDelete                            PROTO    :DWORD, :DWORD
SEDump                              PROTO
SEEmptyStack                        PROTO
SEForget                            PROTO    :DWORD
SEFrame                             PROTO    :DWORD, :DWORD
SEGetGlobalSlotSize                 PROTO
SEGetSlot                           PROTO
SEHexDump                           PROTO
SEInsert                            PROTO    :DWORD, :DWORD
SEInsertFrom                        PROTO    :DWORD, :DWORD
SELeft                              PROTO    :DWORD
SELengthOf                          PROTO    :DWORD
SELength                            PROTO
SEMessage                           PROTO    :DWORD
SEMessage2                          PROTO   
SEMid                               PROTO    :DWORD, :DWORD
SEMoveSlot                          PROTO    :DWORD, :DWORD
SEPop                               PROTO
SEPrependFrom                       PROTO    :DWORD
SEPrepend                           PROTO    :DWORD
SERawHex                            PROTO
SEReadFile                          PROTO    :DWORD, :DWORD
SERecall                            PROTO    :DWORD
SERecallLMR                         PROTO    :DWORD
SEResetLMR                          PROTO
SEReverseBytes                      PROTO
SEReverseWords                      PROTO
SERight                             PROTO    :DWORD
SESetGlobalSlotSize                 PROTO    :DWORD
SESetSlot                           PROTO    :DWORD
SEStatus                            PROTO    :DWORD
SEStore                             PROTO    :DWORD
SESwapSlot                          PROTO    :DWORD, :DWORD
SEUTF16Scan                         PROTO    :DWORD
SEWriteFile                         PROTO    :DWORD
           
;Internal functions
Instantiate                         PROTO    :DWORD
Log_Error                           PROTO    :DWORD, :DWORD, :DWORD, :DWORD
Set_Error                           PROTO    :DWORD
SpaceAtBase                         PROTO
SpaceToTop                          PROTO
Verify                              PROTO
WillFitAtBase                       PROTO    :DWORD
WillFitAtTop                        PROTO    :DWORD
Compare_CheckReferenceSlot          PROTO    :DWORD
Compare_CheckTargetSlots            PROTO    :DWORD, :DWORD
CalculateAndAllocate                PROTO    :DWORD, :DWORD
CopyAndOrForget                     PROTO    :DWORD, :DWORD, :DWORD

Space is allocated (99% of the time) using SysAllocStringByteLen, largely because I was using that in a recent C++ project and because strings are coming in from VB6 using StrPtr.

LibMain looks like this:

Code Select Expand

LibMain proc uses EDI ESI instance:DWORD,reason:DWORD,unused:DWORD
    LOCAL dSlot:Slot
   
    .if reason == DLL_PROCESS_ATTACH
          PUSH instance
          POP hInstance
          MOV EDI, SLOT_COUNT
          MOV ECX, 0
          .while EDI > 0
              PUSH ECX
              MOV ESI, rv(IntMul, ECX, SIZEOF Slot)
              MOV Slots[ESI].instantiated, 0
              MOV Slots[ESI].slotbase, 0
              MOV Slots[ESI].textbase, 0
              MOV Slots[ESI].slotlength, 0
              MOV Slots[ESI].textlength, 0
              MOV Slots[ESI].occupied, 0
            DEC EDI
            POP ECX
            INC ECX
          .endw
          MOV ECX, 64*4
          MOV EStack, rv(SysAllocStringByteLen, 0, ECX)
          MOV EAX, TRUE

    .elseif reason == DLL_PROCESS_DETACH
        MOV ECX, SLOT_COUNT
        .repeat
            PUSH ECX
            MOV ESI, rv(IntMul, ECX, SIZEOF Slot)
            fn SysFreeString,Slots[ESI].slotbase
            POP ECX
            DEC ECX
        .until ECX == 0
        fn SysFreeString, Slots[0].slotbase
        fn SEEmptyStack ;empty error stack
        fn SysFreeString, EStack ;release error stack
       
    .elseif reason == DLL_THREAD_ATTACH

    .elseif reason == DLL_THREAD_DETACH

    .endif

    RET

LibMain endp

So where do I start with all these functions? Well, let's start with SEAppend:

Code Select Expand

SEAppend PROC STDCALL uses EDI ESI sText:DWORD
    LOCAL dNewSpace:DWORD
    LOCAL dSlot:Slot
    LOCAL dMiddleNew:DWORD
    LOCAL dLen:DWORD
   
    ;    make sure that the currentSlot is instantiated
    ;    copy Slots at currentSlot to tempSlot
    ;    if tempSlot is not occupied already
    ;        call SEStore with sText
    ;    else
    ;        if there's room between textbase+textlength and slotbase+slotlength (in tempSlot)
    ;            insert appending text after textbase+textlength
    ;            update textlength
    ;        else (ie. no room)
    ;            allocate new space that is nearest multiple of slotSize > length of occupying string and prepending string
    ;            find middle of space
    ;            subtract from it half of occupying text length and half of prepending text length
    ;            copy occupying text there followed by appending text
    ;            release tempSlot's memory allocation
    ;            copy address of space into tempSlot's pointer
    ;        endif
    ;    endif
    ;    copy tempSlot to Slots at currentSlot
   
    PUSH ECX
    PUSH EDX
   
    MOV EDI, currentSlot
    MOV ESI, rv(IntMul, EDI, SIZEOF Slot)
   
    MOV ECX, Slots[ESI].instantiated
    .if ECX != 1
        fn Instantiate, currentSlot
        fn SEStore, sText
        JMP @F ;keep SEStore's return code alive
    .endif
   
    ;get length of incoming string
    MOV ECX, rv(SysStringByteLen, sText)
    .if ECX == 0
        fn Log_Error, "SEAppend", EDI, E_STORE_NULL, -1
        fn Set_Error, E_ERROR_LOGGED
        JMP @F
    .endif

    MOV dLen, ECX
   
    .if rv(WillFitAtTop, dLen) == 0
        ;registers:
        ;    EAX = address of slotbase + slotlength
        ;    EBX = undefined
        ;    ECX = length of incoming string
        ;    EDX = address after textbase + textlength + new string length
        ;    EDI = number of current slot
        ;    ESI = offset into slots list
       
        ; initialise dSlot
        MOV dSlot.instantiated, 1
        MOV dSlot.occupied, 1
           
        ; ADD length of incoming string to length of existing string
        ADD ECX, Slots[ESI].textlength
       
        ; save this new length in dSlot
        MOV dSlot.textlength, ECX
       
        ; find out what multiple of slotSize will contain this amount of string
        MOV EAX, ECX
        ADD EAX, BUFFER_INCREMENT
       
        ; save new total length
        PUSH ECX
       
        ; save new slot size
        PUSH EAX
       
        ; save new slot size in dSlot's slotlength
        MOV dSlot.slotlength, EAX
       
        ; allocate this much space and save pointer in dSlot
        MOV EDX, rv(SysAllocStringByteLen, 0h, EAX)
        MOV dSlot.slotbase, EDX
       
        ; find middle of allocation, by halving its length and
        ;     adding that to the base address
       
        ; recover new slot size
        POP EAX
        ; divide by two
        SHR EAX, 1
        ; ADD to slotbase, which is in EDX
        ADD EDX, EAX
        ; save address of middle
        MOV dMiddleNew, EDX
       
        ; find place of first insertion:
        ;    divide new total length by two, subtract this figure from slot middle
       
        ; recover new total length and divide by 2
        POP ECX
        SHR ECX, 1
       
        ; subtract from address of middle
        SUB EDX, ECX
       
        ; store this offset into dSlot's textbase
        MOV dSlot.textbase, EDX
       
        ; copy memory from sText for dLen bytes to EDX
       
        ; save EDX as MemCopy will likely change it
        PUSH EDX
       
        ; copy from Slots for textlength to EDX
        fn MemCopy, Slots[ESI].textbase, EDX, Slots[ESI].textlength
       
        ; recover EDX
        POP EDX
       
        ; ADD Slots textlength to EDX to get next insertion point
        ADD EDX, Slots[ESI].textlength
       
        ; insert from Slots
        fn MemCopy, sText, EDX, dLen
       
        ; release old slot
        fn SysFreeString, Slots[ESI].slotbase
       
        ; copy memory from dSlot to current slot
        fn MemCopy, ADDR dSlot, ADDR Slots[ESI], SIZEOF Slot
       
        ; set error flag and move on
        fn Set_Error, E_NO_ERROR
    .else
        ;copy sText to new textbase for ECX bytes
        PUSH ECX
        MOV EDX, Slots[ESI].textbase
        ADD EDX, Slots[ESI].textlength
        fn MemCopy, sText, EDX, ECX
        POP ECX
       
        ;update textbase and textlength
        MOV EAX, Slots[ESI].textlength
        ADD EAX, ECX
        MOV Slots[ESI].textlength, EAX
        MOV Slots[ESI].occupied, 1
        fn Set_Error, E_NO_ERROR
    .endif

@@:
    PUSH EAX
    fn Verify
    POP EAX
   
    POP EDX
    POP ECX
   
    RET

SEAppend endp


Okay, that should keep you going for a little while.

Thanks in advance for all constructive criticism.

Kind regards,
Bruce.