Recent GUI work

[byuu]

Who cares about the source size?

People who want to work with it. I made a few changes that took me _hours_ to find the relevant code to alter. Sometimes I just gave up on some things.

Moreso, my point was about the comment sizes. What if you wanted to understand a group of 20 functions in a file, that in total have half a book worth of comments? I sure wouldn't want to have to read all of that to understand what the functions are doing when a general synopsis would be more than enough.

Then again, I did say nevermind to the point I was trying to make in my previous post. So, nevermind.

[Noxious Ninja]

You can pushad, call, store EAX in RAM, popad. Do what you want with what's in RAM now, even move back to EAX.

So in that case would you just create a return value, then call the function with a pointer to that return value? Or would it be better to use a global variable?

Who cares about the source size?

People who want to work with it. I made a few changes that took me _hours_ to find the relevant code to alter. Sometimes I just gave up on some things.

That's what comments are for.

[Nach]

Well, popad doesn't affect EFL, so you could just do something like:

Code: Select all

cmp al,0
popad
jz .yes
.no:
  ...
  ret
.yes:
  ...
  ret

Yes you can do that as well, in fact I did that several places in ZSNES.

As a PHP coder, I like strcase* more.

What the flying fuck >_<
I swear I remember using stricmp in PHP4... as well as stripos and stristr. Watch those be gone tomorrow too with no mention of stripos/stristr anywhere...

I don't know what to tell you...
PHP basically has all the GNUC string functions and then some of their own.

However, GNUC added strcasestr recently, so perhaps PHP will add that in addition to stristr.

I have not really known PHP to remove functions when they simply came up with a new name for some of them.

That's why patch reviews exist, and that you need like 20 (actually, it's 2 or 3 ) levels of autorizations to get a patch applied to the mozilla trunk.

But I doubt zsnes wants that much bureaucracy

We usually have patches reviewed by one or two people.
We also have enough bureaucracy. In the team I do have to ask the others before I add something a bit ... unusual ... and have gotten responses at times "if you add that, I'm quiting".

[Nach]

You can pushad, call, store EAX in RAM, popad. Do what you want with what's in RAM now, even move back to EAX.

So in that case would you just create a return value, then call the function with a pointer to that return value? Or would it be better to use a global variable?

Depends on what you're doing. If you're going to branch based on the return value, do what byuu and I just said above. If you're going to be using the variable more than that, you can just have the assembly use ZSNES' tempeax variable.

[Noxious Ninja]

OK, so the patch to guiload.inc would then be this, right?

If you're going to be using the variable more than that, you can just have the assembly use ZSNES' tempeax variable.

I don't see any mention of that in the source, how does it work?

Code: Select all

Index: src/gui/guiload.inc
===================================================================
RCS file: /cvsroot/zsnes/zsnes/src/gui/guiload.inc,v
retrieving revision 1.75
diff -u -r1.75 guiload.inc
--- src/gui/guiload.inc	9 Apr 2005 08:40:29 -0000	1.75
+++ src/gui/guiload.inc	29 May 2005 18:26:23 -0000
@@ -1103,9 +1103,16 @@
 .nextentry
     push ecx
     ; check if esi > esi+32
+    pushad
+    add esi,32
+    push esi
+    sub esi,32
+    push esi
     call GUIStringGreater
+    add esp,8
     ; if it is, swap!
     cmp al,0
+    popad
     je near .noswap
     mov ecx,31
 .nextswap
@@ -1150,9 +1157,14 @@
     inc ebx
     inc edx
     ; check if ebx > edx
-    call GUIStringGreater2
+    pushad
+    push edx
+    push ebx
+    call GUIStringGreater
+    add esp,8
     ; if it is, swap!
     cmp al,0
+    popad
     je near .noswapb
     mov eax,[esi]
     mov ebx,[esi+4]
@@ -1219,9 +1231,16 @@
 .nextentry
     push ecx
     ; check if esi > esi+32
+    pushad
+    add esi,32
+    push esi
+    sub esi,32
+    push esi
     call GUIStringGreater
+    add esp,8
     ; if it is, swap!
     cmp al,0
+    popad
     je near .noswap
     mov ecx,31
 .nextswap
@@ -1266,9 +1285,14 @@
     inc ebx
     inc edx
     ; check if ebx > edx
-    call GUIStringGreater2
+    pushad
+    push edx
+    push ebx
+    call GUIStringGreater
+    add esp,8
     ; if it is, swap!
     cmp al,0
+    popad
     je near .noswapb
     mov eax,[esi]
     mov ebx,[esi+4]

[grinvader]

This stack management isn't far from your previous attempt...
Let me show you how I would do it.

(will edit when done)

[Noxious Ninja]

This stack management isn't far from your previous attempt...

It just adds register saving like Nach and Byuu pointed out.

Let me show you how I would do it.

Please do.

[grinvader]

Code: Select all

--- gui/gui_old.asm     2005-05-28 16:22:19.000000000 +0000
+++ gui/gui.asm 2005-05-30 08:36:58.000000000 +0000
@@ -105,7 +105,7 @@
 EXTSYM RestoreSystemVars,GUIBIFIL,GUIHQ2X,GUIHQ3X,GUIHQ4X,firstsaveinc,nssdip1
 EXTSYM nssdip2,nssdip3,nssdip4,nssdip5,nssdip6,SkipMovie,MovieStop,MoviePlay
 EXTSYM MovieRecord,MovieInsertChapter,MovieSeekAhead,MovieSeekBehind
-EXTSYM ResetDuringMovie,MovieDumpRaw
+EXTSYM ResetDuringMovie,MovieDumpRaw,GUIStringGreater,GUIStringGreater2,tempesi
 %ifdef __LINUX__
 EXTSYM numlockptr

Code: Select all

--- gui/guiload_old.inc 2005-04-09 08:40:29.000000000 +0000
+++ gui/guiload.inc     2005-05-30 08:38:41.000000000 +0000
@@ -1102,10 +1102,13 @@
     dec ecx
 .nextentry
     push ecx
-    ; check if esi > esi+32
+    mov [tempesi],esi
+    pushad
+    ; check if [esi] > [esi+32]
     call GUIStringGreater
     ; if it is, swap!
     cmp al,0
+    popad
     je near .noswap
     mov ecx,31
 .nextswap
@@ -1145,14 +1148,13 @@
     dec ecx
 .nextentryb
     push ecx
-    mov ebx,[esi]
-    mov edx,[esi+4]
-    inc ebx
-    inc edx
-    ; check if ebx > edx
+    pushad
+    mov [tempesi],esi
+    ; check if [[esi]+1] > [[esi+4]+1]
     call GUIStringGreater2
     ; if it is, swap!
     cmp al,0
+    popad
     je near .noswapb
     mov eax,[esi]
     mov ebx,[esi+4]
@@ -1218,10 +1220,13 @@
     je near .sort2
 .nextentry
     push ecx
-    ; check if esi > esi+32
+    pushad
+    mov [tempesi],esi
+    ; check if [esi] > [esi+32]
     call GUIStringGreater
     ; if it is, swap!
     cmp al,0
+    popad
     je near .noswap
     mov ecx,31
 .nextswap
@@ -1261,14 +1266,13 @@
     add esi,spcRamcmp
 .nextentryb
     push ecx
-    mov ebx,[esi]
-    mov edx,[esi+4]
-    inc ebx
-    inc edx
-    ; check if ebx > edx
+    pushad
+    mov [tempesi],esi
+    ; check if [[esi]+1] > [[esi+4]+1]
     call GUIStringGreater2
     ; if it is, swap!
     cmp al,0
+    popad
     je near .noswapb
     mov eax,[esi]
     mov ebx,[esi+4]

Code: Select all

--- gui/guiwindp_old.inc        2005-05-28 00:01:43.000000000 +0000
+++ gui/guiwindp.inc    2005-05-30 08:09:45.000000000 +0000
@@ -23,97 +23,6 @@
 ; Window Display Routines
 ; Last button value used = 77

-GUIStringGreater:
-    ; compares string at esi to edi and returns 1 to al if esi is >, else 0
-    push esi
-    cmp word[esi],'.'
-    je .less
-    cmp word[esi+32],'.'
-    je .greater
-    cmp word[esi],'..'
-    je .less
-    cmp word[esi+32],'..'
-    je .greater
-.nextchar
-    cmp byte[esi],0
-    je .less
-    cmp byte[esi+32],0
-    je .greater
-    mov al,[esi]
-    mov cl,[esi+32]
-    cmp al,'a'
-    jb .noucase1
-    cmp al,'z'
-    ja .noucase1
-    sub al,'z'-'Z'
-.noucase1
-    cmp cl,'a'
-    jb .noucase2
-    cmp cl,'z'
-    ja .noucase2
-    sub cl,'z'-'Z'
-.noucase2
-    cmp al,cl
-    jb .less
-    ja .greater
-    inc esi
-    jmp .nextchar
-.less
-    mov al,0
-    jmp .skip
-.greater
-    mov al,1
-.skip
-    pop esi
-    ret
-
-GUIStringGreater2:
-    ; compares string at ebx to edx and returns 1 to al if esi is >, else 0
-    push edx
-    push ebx
-    cmp word[ebx],'.'
-    je .less
-    cmp word[edx],'.'
-    je .greater
-    cmp word[ebx],'..'
-    je .less
-    cmp word[ebx],'..'
-    je .greater
-.nextchar
-    cmp byte[ebx],0
-    je .less
-    cmp byte[edx],0
-    je .greater
-    mov al,[ebx]
-    mov cl,[edx]
-    cmp al,'a'
-    jb .noucase1
-    cmp al,'z'
-    ja .noucase1
-    sub al,'z'-'Z'
-.noucase1
-    cmp cl,'a'
-    jb .noucase2
-    cmp cl,'z'
-    ja .noucase2
-    sub cl,'z'-'Z'
-.noucase2
-    cmp al,cl
-    jb .less
-    ja .greater
-    inc ebx
-    inc edx
-    jmp .nextchar
-.less
-    mov al,0
-    jmp .skip
-.greater
-    mov al,1
-.skip
-    pop ebx
-    pop edx
-    ret
-
 %Macro DrawGUILineSc 2
     mov dword[GUIcolscaleval],%2
     mov edx,%1

Code: Select all

--- Makefile_old.in 2005-04-21 22:14:10.000000000 +0000
+++ Makefile.in 2005-05-30 08:28:18.000000000 +0000
@@ -41,7 +41,7 @@
        ${CPUDIR}/memtable.o ${CPUDIR}/spc700.o ${CPUDIR}/stable.o\
        ${CPUDIR}/table.o ${CPUDIR}/tableb.o ${CPUDIR}/tablec.o

-GUIOBJ=${GUIDIR}/gui.o ${GUIDIR}/menu.o
+GUIOBJ=${GUIDIR}/gui.o ${GUIDIR}/guifuncs.o ${GUIDIR}/menu.o

 VIDEOBJ=${VIDEODIR}/makev16b.o ${VIDEODIR}/makev16t.o ${VIDEODIR}/makevid.o\
        ${VIDEODIR}/mode716.o ${VIDEODIR}/mode716b.o ${VIDEODIR}/mode716d.o\
@@ -188,6 +188,7 @@
        ${GUIDIR}/guimisc.inc ${GUIDIR}/guimouse.inc ${GUIDIR}/guiwindp.inc\
   ${GUIDIR}/guikeys.inc ${GUIDIR}/guicheat.inc\
        ${GUIDIR}/guicombo.inc ${GUIDIR}/guiload.inc macros.mac
+${GUIDIR}/guifuncs.o: ${GUIDIR}/guifuncs.c
 ${GUIDIR}/menu.o: ${GUIDIR}/menu.asm macros.mac
 ${VIDEODIR}/newgfx.o:${VIDEODIR}/newgfx.asm ${VIDEODIR}/vidmacro.mac\
        ${VIDEODIR}/newgfx2.mac ${VIDEODIR}/newgfx.mac macros.mac

New file:

Code: Select all

/*
Copyright (C) 1997-2005 ZSNES Team ( zsKnight, _Demo_, pagefault, Nach )

http://www.zsnes.com
http://sourceforge.net/projects/zsnes

This program is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public License
as published by the Free Software Foundation; either
version 2 of the License, or (at your option) any later
version.

This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
GNU General Public License for more details.

You should have received a copy of the GNU General Public License
along with this program; if not, write to the Free Software
Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/

#ifdef __LINUX__
#include "gblhdr.h"
#define DIR_SLASH "/"
#else
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <zlib.h>
#define DIR_SLASH "\\"
#endif
#include "gblvars.h"

bool dirstringcmp(char *string1, char *string2)
{ // compares two strings - returns true if string1 is >, else 0

if (!strcmp(string1, "."))  { return (false); }
if (!strcmp(string2, "."))  { return (true); }
if (!strcmp(string1, "..")) { return (false); }
if (!strcmp(string2, "..")) { return (true); }

   return (strcasecmp(string1, string2) > 0); // if equal -> false
}

char GUIStringGreater()
{
  char *ptr = (char *)tempesi;

  return ((char)dirstringcmp(ptr, ptr+32));
}

char GUIStringGreater2()
{
  char **ptr = (char **)tempesi;

  return ((char)dirstringcmp((*ptr)+1, (*(ptr+1))+1)); // *(ptr+1) == [esi+4]
}

I agree that this is only temporary. Once the calling code is cleaner, you can fuse the two easily.
But in the meantime, wrapping to a common func from 2 small funcs is easier, safer and gets the job done.

tempesi is a var used to keep backups of esi (durr..), so I used it here.
For GUIStringGreater2, I included the pointer magic done in the asm to set the proper addresses in ebx/edx, so it looks weirder than GUIStringGreater1 but does the same.

[Noxious Ninja]

I see how that would be faster, but I guess I don't understand why you consider that easier and safer.

[byuu]

If Ninja's first post is to be believed, then:
return (strcasecmp(string1, string2) > 0); // if equal -> false
should be:
return (strcasecmp(string1, string2) >= 0); // if equal -> false

But eh. I don't know/care what the asm routine did.

Also, referencing an outside assembly variable (and a copy of an x86-specific register at that) kind of defeats the whole purpose of using c/c++ in the first place, don't you think?

If you guys succeed and get 100% of this ported to c/c++, and then (possibly) try and port it to a non-x86 platform, that's going to look incredibly silly, isn't it? Though you'll probably not finish the porting 100% anyway, so I guess that's a moot point already.

[Noxious Ninja]

If Ninja's first post is to be believed, then:
return (strcasecmp(string1, string2) > 0); // if equal -> false
should be:
return (strcasecmp(string1, string2) >= 0); // if equal -> false

But eh. I don't know/care what the asm routine did.

As I understand it, the second one is more efficient, as if the first string is greater, we swap. If the two are equal, then we should return less (0) so there is no swap. This eliminates a pointless swap.

EDIT: My brain is muddled right now. I'm not sure if what i just said is true. Bah.

EDIT2: What I said was right, but grin's version, not mine, is correct.

[grinvader]

@NN: Easier because you can easily (duh) follow stack operations. If you read before/after, you'll see eax contents aren't critical, so pushad/popad is the way.
Safer because I dislike direct esp manipulation. I find it unintuitive, and it's very easy to b0rk everything when you change the code after not working on it for a while.

Also, referencing an outside assembly variable (and a copy of an x86-specific register at that) kind of defeats the whole purpose of using c/c++ in the first place, don't you think?

If you guys succeed and get 100% of this ported to c/c++, and then (possibly) try and port it to a non-x86 platform, that's going to look incredibly silly, isn't it? Though you'll probably not finish the porting 100% anyway, so I guess that's a moot point already.

This is neither optimized nor anything.

It's a working, safe yet temporary port, waiting to port the calling function for optimum code.
Once calling function is ported, funcs under it are cleaned.
My port of loadstate has 3 mini-funcs wrapping to the main ported func until we port the funcs above, for example.

Once code is 100% clean C, temps won't be used at all, no more wrapping either, only direct calls with proper C parameters.

[Noxious Ninja]

@NN: Easier because you can easily (duh) follow stack operations. If you read before/after, you'll see eax contents aren't critical, so pushad/popad is the way.

I did use pushad/popad, after Byuu and Nach pointed that solution out.

Safer because I dislike direct esp manipulation. I find it unintuitive, and it's very easy to b0rk everything when you change the code after not working on it for a while.

That's true enough.

So how about we change the function prototype to "bool __stdcall GUIStringGreater" and use the following. This doesn't modify esi, doesn't modify esp, and still gets us down to one function.

EDIT: Even better, thanks to this article.

Code: Select all

Index: src/macros.mac
===================================================================
RCS file: /cvsroot/zsnes/zsnes/src/macros.mac,v
retrieving revision 1.51
diff -u -r1.51 macros.mac
--- src/macros.mac	19 Feb 2005 22:11:34 -0000	1.51
+++ src/macros.mac	31 May 2005 01:49:45 -0000
@@ -156,3 +156,26 @@
 %endmacro
 
 
+; ccall FuncName, param1, param2, param 3... ;C: Last-1st, stack cleanup
+%macro ccall 2-*                        
+%define _j %1
+%assign __params %0-1
+%rep %0-1
+    %rotate -1
+    push %1
+%endrep
+    call _j
+    %assign __params __params * 4
+    add esp, __params
+%endmacro
+
+
+; stdcall FuncName, param1, param2, param 3... ;StdCall: last-1st, no cleanup
+%macro stdcall 2-*          
+%define _j %1
+%rep %0-1
+    %rotate -1
+    push %1
+%endrep
+    call _j
+%endmacro

Code: Select all

Index: src/gui/guiload.inc
===================================================================
RCS file: /cvsroot/zsnes/zsnes/src/gui/guiload.inc,v
retrieving revision 1.75
diff -u -r1.75 guiload.inc
--- src/gui/guiload.inc	9 Apr 2005 08:40:29 -0000	1.75
+++ src/gui/guiload.inc	31 May 2005 01:53:43 -0000
@@ -1103,9 +1103,13 @@
 .nextentry
     push ecx
     ; check if esi > esi+32
-    call GUIStringGreater
+    pushad
+    mov dword[tempesi],esi
+    add dword[tempesi],32
+    stdcall GUIStringGreater, esi, dword[tempesi]
     ; if it is, swap!
     cmp al,0
+    popad
     je near .noswap
     mov ecx,31
 .nextswap
@@ -1150,9 +1154,11 @@
     inc ebx
     inc edx
     ; check if ebx > edx
-    call GUIStringGreater2
+    pushad
+    stdcall GUIStringGreater, ebx, edx
     ; if it is, swap!
     cmp al,0
+    popad
     je near .noswapb
     mov eax,[esi]
     mov ebx,[esi+4]
@@ -1219,9 +1225,13 @@
 .nextentry
     push ecx
     ; check if esi > esi+32
-    call GUIStringGreater
+    pushad
+    mov dword[tempesi],esi
+    add dword[tempesi],32
+    stdcall GUIStringGreater, esi, dword[tempesi]
     ; if it is, swap!
     cmp al,0
+    popad
     je near .noswap
     mov ecx,31
 .nextswap
@@ -1266,9 +1276,11 @@
     inc ebx
     inc edx
     ; check if ebx > edx
-    call GUIStringGreater2
+    pushad
+    stdcall GUIStringGreater, ebx, edx
     ; if it is, swap!
     cmp al,0
+    popad
     je near .noswapb
     mov eax,[esi]
     mov ebx,[esi+4]

[Noxious Ninja]

Well?

"O.K."

or

"SUXXOR"

?

[grinvader]

I'm not exactly fond of this method.

But come on, you know enough to start helping. Join the IRC channel and start crackin'.