From: Nigel S. a. F. S. <ni...@ni...> - 2003-12-17 13:22:39
|
Hello all, A few other minor changes for FreeGLUT on Cygwin. Mainly this is to clarify to the compiler between X11 and Win32 targets, and areas that gcc 3.3.1 finds objectionable in paranoid mode. 1. The target host is Win32 if the compiler is either MSVC, Cygwin or MingW32 (freeglut_internal.h) #if !defined(_WIN32) # define TARGET_HOST_UNIX_X11 1 # define TARGET_HOST_WIN32 0 #else # define TARGET_HOST_UNIX_X11 0 # define TARGET_HOST_WIN32 1 #endif -> #if defined(_MSC_VER) || defined(__CYGWIN__) || defined(__MINGW32__) # define TARGET_HOST_UNIX_X11 0 # define TARGET_HOST_WIN32 1 #else # define TARGET_HOST_UNIX_X11 1 # define TARGET_HOST_WIN32 0 #endif 2. strdup is properly supported on Cygwin and MingW32 (freeglut_internal.h) #if defined(_MSC_VER) #define strdup _strdup #endif 3. The joystick code tends to use the WIN32 define instead of TARGET_HOST_WIN32 (freeglut_joystick.c) 4. The stroke and bitmap font interface consistency for WIN32 targets: (freeglut_std.h) #if defined(WIN32) -> #if defined(_MSC_VER) || defined(__CYGWIN__) || defined(__MINGW32__) 5. gcc is unhappy about the following manner of initialisation: (freeglut_cursor.c) POINT coords = { x, y }; -> POINT coords; coords.x = x; coords.y = y; 6. gcc is unhappy about (char *) and (unsigned char *) mismatch in calls to glutBitmapString and glutBitmapLength. (freeglut_menu.c) glutBitmapString( FREEGLUT_MENU_FONT, menuEntry->Text); -> glutBitmapString( FREEGLUT_MENU_FONT, (unsigned char *) menuEntry->Text); 7. gcc is unhappy about (char *) and (unsigned char *) mismatch in calls to strlen. (freeglut_font.c) int numchar = strlen( string ); -> int numchar = strlen( (char *) string ); 8. The XParseGeometry interface has a signed/unsigned mismatch for the 3rd and 4th parameters. (freeglut_init.c) static int XParseGeometry ( const char *string, int *x, int *y, unsigned int *width, /* RETURN */ unsigned int *height) /* RETURN */ -> static int XParseGeometry ( const char *string, int *x, int *y, int *width, /* RETURN */ int *height) /* RETURN */ 9. fgState.FPSInterval is an unsigned integer, mismatch with "%d" sscanf string. (freeglut_init.c) sscanf( fps, "%d", &fgState.FPSInterval ); -> sscanf( fps, "%u", &fgState.FPSInterval ); 10. gcc suggests explicit braces to avoid ambiguous `else' (freeglut_main.c) if( GetSystemMetrics( SM_SWAPBUTTON ) ) if( button == GLUT_LEFT_BUTTON ) button = GLUT_RIGHT_BUTTON; else if( button == GLUT_RIGHT_BUTTON ) button = GLUT_LEFT_BUTTON; -> if (GetSystemMetrics(SM_SWAPBUTTON)) { if (button == GLUT_LEFT_BUTTON) button = GLUT_RIGHT_BUTTON; else if (button == GLUT_RIGHT_BUTTON) button = GLUT_LEFT_BUTTON; } 11. gcc suggests parentheses around assignment used as truth value (freeglut_main.c) [I do find this improves the code, personally] SFG_Timer *timer; while( timer = fgState.Timers.First ) { -> while (fgState.Timers.First) { SFG_Timer *timer = fgState.Timers.First; 12. Similar issue to (11) in freeglut_structure.c in fgCloseWindows, fgDestroyWindow, fgDestroyMenu, fgDestroyStructure, fgListAppend, fgListRemove Cheers, Nigel |
From: Nigel S. a. F. S. <ni...@ni...> - 2003-12-17 14:10:17
|
Oops, two more... 13. The target host is Win32 if the compiler is either MSVC, Cygwin or MingW32 (freeglut_std.h) #if defined(WIN32) -> #if defined(_MSC_VER) || defined(__CYGWIN__) || defined(__MINGW32__) 14. GLUI applications that mix double buffered and single buffered windows exhibit artifacts on Win32. This is observed in MSVC, Cygwin and Mingw32 binaries. (freeglut_window.c in fgOpenWindow) window->Window.DoubleBuffered = ( fgState.DisplayMode & GLUT_DOUBLE ) ? 1 : 0; if ( ! window->Window.DoubleBuffered ) { glDrawBuffer ( GL_FRONT ); glReadBuffer ( GL_FRONT ); } fgSetWindow( window ); -> fgSetWindow( window ); window->Window.DoubleBuffered = ( fgState.DisplayMode & GLUT_DOUBLE ) ? 1 : 0; if ( ! window->Window.DoubleBuffered ) { glDrawBuffer ( GL_FRONT ); glReadBuffer ( GL_FRONT ); } The call to fgSetWindow needs to be _before_ any OpenGL calls becaue fgSetWindow makes the OpenGL context current. |
From: Chris P. <cj...@lo...> - 2003-12-17 14:42:48
|
On Thu, Dec 18, 2003 at 12:24:10AM +1100, Nigel Stewart and Fiona Smith wrote: > 6. gcc is unhappy about (char *) and (unsigned char *) mismatch in > calls to glutBitmapString and glutBitmapLength. > (freeglut_menu.c) > > glutBitmapString( FREEGLUT_MENU_FONT, menuEntry->Text); > -> > glutBitmapString( FREEGLUT_MENU_FONT, (unsigned char *) > menuEntry->Text); They only way I seem to be able to get gcc to give me that warning is with -pedantic. But if I use that option I get lost of the following warnings with the new callback macros: warning: ISO C forbids use of cast expressions as lvalues warning: ISO C forbids initialization between function pointer and void *' > > 7. gcc is unhappy about (char *) and (unsigned char *) mismatch in > calls to strlen. (freeglut_font.c) > > int numchar = strlen( string ); > -> > int numchar = strlen( (char *) string ); > The code should be rewritten to remove those strlen()s. They are inefficient. -- Christopher John Purnell | I thought I'd found a reason to live http://www.lost.org.uk/ | Just like before when I was a child --------------------------| Only to find that dreams made of sand What gods do you pray to? | Would just fall apart and slip through my hands |
From: Nigel S. a. F. S. <ni...@ni...> - 2003-12-17 14:47:02
|
> They only way I seem to be able to get gcc to give me that warning is with > -pedantic. But if I use that option I get lost of the following warnings > with the new callback macros: > > warning: ISO C forbids use of cast expressions as lvalues > warning: ISO C forbids initialization between function pointer and void *' Yes, I didn't want to grapple with that issue... But now that you mention it... :-) |
From: Norman V. <nh...@ca...> - 2003-12-17 14:57:07
|
> > > They only way I seem to be able to get gcc to give me that warning is with > > -pedantic. FWIW - I generally use '-Wall -W' instead of '-pedantic' This should give you all of error/warning messages you need to produce 'correct' code without touching some of the "pedantic" issues Note that '-W' turns on *lots* of error checking in addition to what '-Wall' does Norman |
From: Richard R. <sf...@ol...> - 2003-12-18 10:03:47
|
On Wed, Dec 17, 2003 at 10:03:27AM -0500, Norman Vine wrote: [...] > FWIW - I generally use > > '-Wall -W' instead of '-pedantic' I didn't know about this. I thought that I'd read about all of the warning options in gcc. Thanks. (^& -- "I probably don't know what I'm talking about." http://www.olib.org/~rkr/ |
From: Richard R. <sf...@ol...> - 2003-12-18 10:32:57
|
On Thu, Dec 18, 2003 at 01:48:40AM +1100, Nigel Stewart and Fiona Smith wrote: > > >They only way I seem to be able to get gcc to give me that warning is with > >-pedantic. But if I use that option I get lost of the following warnings > >with the new callback macros: > > > >warning: ISO C forbids use of cast expressions as lvalues > >warning: ISO C forbids initialization between function pointer and void *' > > Yes, I didn't want to grapple with that issue... > But now that you mention it... :-) I saw something that reminded me of this limitation a couple of weeks ago, and then I wondered that the normal compile didn't spew errors on the macros that I wrote. I decided that I must have mis-remembered the ANSI/ISO rules. Since it didn't flag with "-Wall", I just ignored it. If it is spewing lots of warnings for people with "-Wall -W", then it needs to be fixed. I think it should be easy to do so by making a new macro. I believe that a SET_WCB() macro is the way to go. Here is a first-cut at it, to be added to freeglut_internal.h: /* * SET_WCB() is used as: * * SET_WCB( window, Visibility, func ); * * ...where {window} is the freeglut window to set the callback, * {Visibility} is the window-specific callback to set, * {func} is a function-pointer. * * Originally, {FETCH_WCB( ... ) = func} was rather sloppily used, * but this can cause warnings because the FETCH_WCB() macro type- * casts its result, and an lvalue is not supposed to be type-cast. * * XXX Note that there is no type-checking to make sure that {func} is * XXX a suitable type. We could add a safety-check of the form: * XXX * XXX if( FETCH_WCB( ... ) != func ) * XXX ... * XXX * XXX ...is this desired? */ #define SET_WCB(window,cbname,func) \ (((window).CallBacks[CB_ ## cbname]) = func) This should be harmless, and solve the warnings, so if there are no objections I'll put it in later today. -- "I probably don't know what I'm talking about." http://www.olib.org/~rkr/ |
From: Richard R. <sf...@ol...> - 2003-12-19 01:00:57
|
> I believe that a SET_WCB() macro is the way to go. Here is a first-cut > at it, to be added to freeglut_internal.h: [...] I just committed this, with a type-checking wrapper, along with changes to the places where FETC_WCB() was used as an lvalue. I hope that this takes care of those warnings. (^&. -- "I probably don't know what I'm talking about." http://www.olib.org/~rkr/ |
From: Nigel S. a. F. S. <ni...@ni...> - 2003-12-30 03:13:06
|
> 8. The XParseGeometry interface has a signed/unsigned mismatch for > the 3rd and 4th parameters. (freeglut_init.c) > > static int XParseGeometry ( > const char *string, > int *x, > int *y, > unsigned int *width, /* RETURN */ > unsigned int *height) /* RETURN */ > > -> > > static int XParseGeometry ( > const char *string, > int *x, > int *y, > int *width, /* RETURN */ > int *height) /* RETURN */ Given that XParseGeometry is an interface from Xlib, shall we retain width and height as unsigned int and wrap the call with some extra handling: int x,y; unsigned int width,height; int mask = XParseGeometry( geometry, &x, &y, &width, &height ); fgState.Position.X = x; fgState.Position.Y = y; fgState.Size.X = width; fgState.Size.Y = height; It's a litte more long-winded this way, but it resolves the last gcc warnings in -Wall -pendantic mode for Cygwin and Mingw32. Nigel |
From: Richard R. <sf...@ol...> - 2003-12-30 05:25:13
|
On Tue, Dec 30, 2003 at 02:14:44PM +1100, Nigel Stewart and Fiona Smith wrote: > > >8. The XParseGeometry interface has a signed/unsigned mismatch for > > the 3rd and 4th parameters. (freeglut_init.c) > > > >static int XParseGeometry ( > > const char *string, > > int *x, > > int *y, > > unsigned int *width, /* RETURN */ > > unsigned int *height) /* RETURN */ Everything I have checked says that this is the definition. > >-> > > > >static int XParseGeometry ( > > const char *string, > > int *x, > > int *y, > > int *width, /* RETURN */ > > int *height) /* RETURN */ I am not sure where this is coming from. I cannot find it. (Or do you mean that GCC is telling you that this is how the function is being used?) I do not get any warnings (and I have enabled freeglut's warnings, which does "-Werror" as well, I believe). All that I can think of is that this might not be flagged for me since I do not have "-pedantic" turned on. > Given that XParseGeometry is an interface from Xlib, > shall we retain width and height as unsigned int > and wrap the call with some extra handling: > > int x,y; > unsigned int width,height; > > int mask = XParseGeometry( geometry, &x, &y, &width, &height ); > > fgState.Position.X = x; > fgState.Position.Y = y; > fgState.Size.X = width; > fgState.Size.Y = height; You can drop the {x, y} variables and directly initialize them, yes? unsigned int width, height; int mask = XParseGeometry( geometry, &fgState.Position.X, &fgState.Position.Y, &width, & height ); fgState.Size.X = width; fgState.Size.Y = height; ...that saves a little bit of verbosity. Not much, I'll grant, but... An alternative might be to make the freeglut internal structure a little more rational: Negative positions make some sense, but negative dimensions do not. I believe that I used negative dimensions as an out-of-bounds value in one place, but that could be replaced with 0,0 dimensions, or ~0,~0. What other fallout would there be to making the {Size} struct have unsigned parts? -- "I probably don't know what I'm talking about." http://www.olib.org/~rkr/ |
From: Nigel S. a. F. S. <ni...@ni...> - 2003-12-30 05:32:17
|
According to XLib... >>>static int XParseGeometry ( >>> const char *string, >>> int *x, >>> int *y, >>> unsigned int *width, /* RETURN */ >>> unsigned int *height) /* RETURN */ > > Everything I have checked says that this is the definition. Proposal #1... >>>-> >>> >>>static int XParseGeometry ( >>> const char *string, >>> int *x, >>> int *y, >>> int *width, /* RETURN */ >>> int *height) /* RETURN */ Proposal #2... >> int x,y; >> unsigned int width,height; >> >> int mask = XParseGeometry( geometry, &x, &y, &width, &height ); >> >> fgState.Position.X = x; >> fgState.Position.Y = y; >> fgState.Size.X = width; >> fgState.Size.Y = height; Proposal #3... > You can drop the {x, y} variables and directly initialize them, yes? > > unsigned int width, height; > int mask = XParseGeometry( geometry, > &fgState.Position.X, &fgState.Position.Y, > &width, & height ); > fgState.Size.X = width; > fgState.Size.Y = height; > > > ...that saves a little bit of verbosity. Not much, I'll grant, but... I'm fine with #2 or #3... This was something I didn't want to commit without broader discussion. > An alternative might be to make the freeglut internal structure a little > more rational: Negative positions make some sense, but negative dimensions > do not. I believe that I used negative dimensions as an out-of-bounds > value in one place, but that could be replaced with 0,0 dimensions, or > ~0,~0. What other fallout would there be to making the {Size} struct > have unsigned parts? Probably more of the same, in terms of signed/unsigned issues. :-) Nigel |
From: Richard R. <sf...@ol...> - 2003-12-30 07:26:58
|
On Tue, Dec 30, 2003 at 04:33:54PM +1100, Nigel Stewart and Fiona Smith wrote: [...] > Proposal #2... > > >> int x,y; > >> unsigned int width,height; > >> > >> int mask = XParseGeometry( geometry, &x, &y, &width, &height ); > >> > >> fgState.Position.X = x; > >> fgState.Position.Y = y; > >> fgState.Size.X = width; > >> fgState.Size.Y = height; This is a fine solution. I favor reducing the number of variables if only {widht, height} are required. > Proposal #3... > > >You can drop the {x, y} variables and directly initialize them, yes? [...] > > I'm fine with #2 or #3... This was something I didn't want to commit > without broader discussion. IMHO (speaking as all of 1/12th or 1/13th of the freeglut developers), either #2 or #3 could be committed without discussion. Though a comment on the list might be nice. I would rather #3 if only to reduce the number of variables being introduced. > >An alternative might be to make the freeglut internal structure a little > >more rational: Negative positions make some sense, but negative dimensions > >do not. I believe that I used negative dimensions as an out-of-bounds > >value in one place, but that could be replaced with 0,0 dimensions, or > >~0,~0. What other fallout would there be to making the {Size} struct > >have unsigned parts? > > Probably more of the same, in terms of signed/unsigned issues. :-) Maybe. But in the long run, I think that it is more desirable, since it reflects what the data values can realistically do. You shouldn't have negative widths or heights for windows. I'm a fan of choosing the right types for the right job, and signed width/height doesn't make much sense. (^& I am not saying that it's necessarily worth the effort, mind... -- "I probably don't know what I'm talking about." http://www.olib.org/~rkr/ |
From: Richard R. <sf...@ol...> - 2003-12-30 10:59:43
|
Hm... I thought that I committed the fgSleepForEvents() code last night. Sorry, but it's in there now. (In fact, I remember writing the commit log, so it's weird...but "cvs diff" showed that the changes still weren't in.) Sorry for any angst caused by the excess delay. I can't explain it, but it should *now* be in there. -- "I probably don't know what I'm talking about." http://www.olib.org/~rkr/ |
From: Richard R. <sf...@ol...> - 2003-12-30 11:11:45
|
I don't understand some recent changes: --- src/freeglut_init.c 30 Dec 2003 02:14:54 -0000 1.27 +++ src/freeglut_init.c 28 Nov 2003 19:19:54 -0000 1.26 @@ -519,13 +519,9 @@ const char *fps = getenv( "GLUT_FPS" ); if( fps ) { - int interval; - sscanf( fps, "%d", &interval ); - - if( interval <= 0 ) - fgState.FPSInterval = 5000; /* 5000 millisecond default */ - else - fgState.FPSInterval = interval; + sscanf( fps, "%d", &fgState.FPSInterval ); + if( fgState.FPSInterval <= 0 ) + fgState.FPSInterval = 5000; /* 5000 milliseconds */ } } ...what was gained by that? (The old code is marked by '+', the new by '-'.) It adds a variable, as well as a few lines of code. Was it just personal taste? (I found the old form clearer, personally. (^&) Or is there some warning that was silenced by this? -- "I probably don't know what I'm talking about." http://www.olib.org/~rkr/ |
From: Nigel S. a. F. S. <ni...@ni...> - 2003-12-30 11:53:40
|
> const char *fps = getenv( "GLUT_FPS" ); > if( fps ) > { > - int interval; > - sscanf( fps, "%d", &interval ); > - > - if( interval <= 0 ) > - fgState.FPSInterval = 5000; /* 5000 millisecond default */ > - else > - fgState.FPSInterval = interval; > + sscanf( fps, "%d", &fgState.FPSInterval ); > + if( fgState.FPSInterval <= 0 ) > + fgState.FPSInterval = 5000; /* 5000 milliseconds */ > } > } > > ...what was gained by that? (The old code is marked by '+', the new by '-'.) gcc was complaining about GLuint fgState.FPSInterval being treated as a signed integer in the sscanf control string. In fact, the code was broken a GLuint will never be negative and the 5000 millisecond default would never be applied. In the new code, the environment variable is converted to an int interval, checked for negativity. I suppose the sscanf could be converted to an atoi, but apart from that I think the new code correctly implements the original intention - as well as resolve gcc -Wall -pendantic paranoia. Consider what would happen in 2.2.0 if GLUT_FPS is -1. Nigel |
From: Richard R. <sf...@ol...> - 2003-12-30 14:56:49
|
On Tue, Dec 30, 2003 at 10:55:18PM +1100, Nigel Stewart and Fiona Smith wrote: > > > const char *fps = getenv( "GLUT_FPS" ); > > if( fps ) > > { > >- int interval; > >- sscanf( fps, "%d", &interval ); > >- > >- if( interval <= 0 ) > >- fgState.FPSInterval = 5000; /* 5000 millisecond default > >*/ > >- else > >- fgState.FPSInterval = interval; > >+ sscanf( fps, "%d", &fgState.FPSInterval ); > >+ if( fgState.FPSInterval <= 0 ) > >+ fgState.FPSInterval = 5000; /* 5000 milliseconds */ > > } > > } > > > >...what was gained by that? (The old code is marked by '+', the new by > >'-'.) > > gcc was complaining about GLuint fgState.FPSInterval > being treated as a signed integer in the sscanf > control string. In fact, the code was broken > a GLuint will never be negative and the 5000 > millisecond default would never be applied. Okay. (I personally think that gcc has no business guessing at what sscanf() does. It's not as if the standard library is part of the *language*. (^& But, fixing the code brokeness is good, in any case. Thanks for the explanation. -- "I probably don't know what I'm talking about." http://www.olib.org/~rkr/ |
From: Richard R. <sf...@ol...> - 2003-12-30 11:41:59
|
I just noticed that recent changed put in #ifdef TARGET_HOST_WIN32. (I did something analogous with TARGET_HOST_UNIX_X11 a ways back; (^&.) Should be #if, not #ifdef, since those symbols are defined for all targets, but take on different values according to the system type. I'm fixing that, now... -- "I probably don't know what I'm talking about." http://www.olib.org/~rkr/ |
From: Nigel S. a. F. S. <ni...@ni...> - 2003-12-30 11:54:17
|
> I just noticed that recent changed put in #ifdef TARGET_HOST_WIN32. > (I did something analogous with TARGET_HOST_UNIX_X11 a ways back; (^&.) Oops, sorry! Nigel |