From: Fay J. F C. AAC/W. <joh...@eg...> - 2004-09-02 15:25:12
|
Gentlemen, I'm giving the "freeglut" code another bit of a scrub (I've been comparing it with OpenGLUT) and would like some discussion on some proposed changes. The first proposed change is that there are still a bunch of three-line comments whose first line is just "/*" and whose last line is "*/". I propose to shrink these to one-line comments when they are inside functions so that the code as a whole loses a few lines. Comments like this before functions have the secondary effect of setting off one function from another so that you can tell where one ends and the next begins. The second proposed change is specific to joysticks: I suggest that we not initialize the joysticks until and unless the application has defined a joystick callback. This is what OpenGLUT does, and I think it's a good idea. It costs one variable in "fgState" and a check in the joystick callback definition function; it saves us a rather lengthy function call on initialization in the cases where the application does not care about joysticks. The third proposed change is to make "fg" functions that are invoked only from the file in which they are defined "fgh" and declare them static. I don't see this having any impact on performance and I see it reducing the namespace size. I also see it making the code simpler to maintain because when we see a static function we know it isn't being referred to from somewhere else. A fourth proposed change concerns warning and error messages. It is really several changes: - Remove trailing "\n" characters from the "fgError" and "fgWarning" format strings; those functions put them on automatically. - Change calls of the style 'fgWarning ( "%s", "<string>" )' to 'fgWarning ( "<string>" )' -- it's a bit simpler and faster - Remove the "freeglut_assert_ready" and "freeglut_assert_window" invocations from places where the code can do something reasonable even if the assertion would fail - Change "fgError" to "fgWarning" (so execution can continue) in cases where an error is not particularly fatal Comments on any of these? I'll be putting more in as I dig through what I have. John F. Fay joh...@eg... 850-729-6330 |
From: Brian P. <bri...@tu...> - 2004-09-02 16:27:10
|
Fay John F Contr AAC/WMG wrote: > Gentlemen, > > I'm giving the "freeglut" code another bit of a scrub (I've been > comparing it with OpenGLUT) and would like some discussion on some > proposed changes. > > The first proposed change is that there are still a bunch of > three-line comments whose first line is just "/*" and whose last line is > "*/". I propose to shrink these to one-line comments when they are > inside functions so that the code as a whole loses a few lines. > Comments like this before functions have the secondary effect of setting > off one function from another so that you can tell where one ends and > the next begins. > > The second proposed change is specific to joysticks: I suggest > that we not initialize the joysticks until and unless the application > has defined a joystick callback. This is what OpenGLUT does, and I > think it's a good idea. It costs one variable in "fgState" and a check > in the joystick callback definition function; it saves us a rather > lengthy function call on initialization in the cases where the > application does not care about joysticks. > > The third proposed change is to make "fg" functions that are > invoked only from the file in which they are defined "fgh" and declare > them static. I don't see this having any impact on performance and I > see it reducing the namespace size. I also see it making the code > simpler to maintain because when we see a static function we know it > isn't being referred to from somewhere else. > > A fourth proposed change concerns warning and error messages. > It is really several changes: > - Remove trailing "\n" characters from the "fgError" and > "fgWarning" format strings; those functions put them on automatically. > > - Change calls of the style 'fgWarning ( "%s", > "<string>" )' to 'fgWarning ( "<string>" )' -- it's a bit simpler and faster > > - Remove the "freeglut_assert_ready" and > "freeglut_assert_window" invocations from places where the code can do > something reasonable even if the assertion would fail > > - Change "fgError" to "fgWarning" (so execution can > continue) in cases where an error is not particularly fatal > > Comments on any of these? > > I'll be putting more in as I dig through what I have. I'm just lurking nowadays but this all sounds good. Though I'd probably be conservative when removing assertions. Even if you can recover from what would have been a failed assertion, it's sometimes a sign of a real problem elsewhere. -Brian |
From: Richard R. <sf...@ol...> - 2004-09-02 18:16:13
|
[...] > > The second proposed change is specific to joysticks: I suggest= =20 > >that we not initialize the joysticks until and unless the application=20 > >has defined a joystick callback. This is what OpenGLUT does, and I=20 > >think it's a good idea. It costs one variable in "fgState" and a check= =20 > >in the joystick callback definition function; it saves us a rather=20 > >lengthy function call on initialization in the cases where the=20 > >application does not care about joysticks. The main reason that I implemented that in OpenGLUT was to shut up warnings about missing joysticks when running, e.g., the shapes demo. Programs that don't use a resource shouldn't get uppity about the resource not being available. > > The third proposed change is to make "fg" functions that are=20 > >invoked only from the file in which they are defined "fgh" and declare= =20 > >them static. I don't see this having any impact on performance and I=20 > >see it reducing the namespace size. I also see it making the code=20 > >simpler to maintain because when we see a static function we know it=20 > >isn't being referred to from somewhere else. If the commits are done at fine granularity, I should catch any that you make {static} that we missed in OpenGLUT. But if they go in in one or two large batches, I'd appreciate hearing about any that you notice that OpenGLUT missed. [...] > > - Remove the "freeglut_assert_ready" and=20 > >"freeglut_assert_window" invocations from places where the code can do= =20 > >something reasonable even if the assertion would fail I think that we discussed this some last year on the freeglut lists. The issue that I have (mentioned recently in openglut-devel, so I'll just cut to the chase) is whether assert() is the right mechanism. In many cases in freeglut (and OpenGLUT) I think that it is not. This is not quite the same issue as wehther a reasonable continuation exists. --=20 "I probably don't know what I'm talking about." http://www.olib.org/~rkr/ |
From: Peter M. <pj...@cs...> - 2004-09-02 22:19:29
|
John, > The first proposed change is that there are still a bunch of > three-line comments whose first line is just "/*" and whose last line is > "*/". I propose to shrink these to one-line comments when they are inside > functions so that the code as a whole loses a few lines. Comments like this > before functions have the secondary effect of setting off one function from > another so that you can tell where one ends and the next begins. Simple to do, and probably worth it to just help de-bulk the files. > The second proposed change is specific to joysticks: I suggest that > we not initialize the joysticks until and unless the application has defined > a joystick callback. This is what OpenGLUT does, and I think it's a good > idea. It costs one variable in "fgState" and a check in the joystick > callback definition function; it saves us a rather lengthy function call on > initialization in the cases where the application does not care about > joysticks. Logical idea, as long as there are no other hidden side effects. > The third proposed change is to make "fg" functions that are invoked > only from the file in which they are defined "fgh" and declare them static. > I don't see this having any impact on performance and I see it reducing the > namespace size. I also see it making the code simpler to maintain because > when we see a static function we know it isn't being referred to from > somewhere else. I'm a <big> fan of the static function. I don't think enough programmers use"static" on function names. Furthermore, havinf <some> sort of nomenclature for differentiating between static and public functions is also a great idea. Although this won't work for FreeGlut, I personally use an all capital prefixes for public functions, and the same prefix in all lower case for static functions. Forexample, suppose I have a source file called "GuiWindowFuncs.c". I would prefix public function with "GWF_" and static functions with "gwf_". It gives me a way to instantly know what function are static (i.e. local) and which are public, and when the public functions are in another source file, I know what file they are in because the prefix is always related to the name of the source file. > A fourth proposed change concerns warning and error messages. It is > really several changes: > - Remove trailing "\n" characters from the "fgError" and > "fgWarning" format strings; those functions put them on automatically. > - Change calls of the style 'fgWarning ( "%s", "<string>" )' > to 'fgWarning ( "<string>" )' -- it's a bit simpler and faster > - Remove the "freeglut_assert_ready" and > "freeglut_assert_window" invocations from places where the code can do > something reasonable even if the assertion would fail > - Change "fgError" to "fgWarning" (so execution can > continue) in cases where an error is not particularly fatal I know there is always a desire to remove assertions from functions that have been working for years or that have error handling code in them already. However, the assertions cost nothing for the release build, and they act as permanent insurance in debug builds. In this case, I would leave them since you never know when they may prove their worth by firing off someday in response to some innocuous coding change somewhere. If they save you debugging time when it happens, then they are worth keeping. I have no opinion on the string changes, but I do agree that more liberal use of warnings for situations that aren't actually critical is a good idea. Thanks, PeterM |
From: Richard R. <sf...@ol...> - 2004-09-06 07:38:17
|
On Thu, Sep 02, 2004 at 03:16:04PM -0700, Peter Montgomery wrote: [...] > > - Remove the "freeglut_assert_ready" and > > "freeglut_assert_window" invocations from places where the code can do > > something reasonable even if the assertion would fail [...] > I know there is always a desire to remove assertions from functions that > have been working for years or that have error handling code in them > already. However, the assertions cost nothing for the release build, and I think that this relates to the "assert abuse" comments that I attached to many assert() uses in OpenGLUT. I have personally not seen or felt the desire to remove an assert() just because "the code seems to work". I cannot speak for anyone else, of course. The rationale has nothing to do with a false sense of security (and it surely would be false). Rather it is simply that there are many places in freeglut where assert() is abused as if it were a general error-checking mechanism. More is explained on the openglut-devel list, where I can actually speak for a developer (myself). OpenGLUT inherited the freeglut uses of assert(), and I consider this an issue requiring solution. --=20 "I probably don't know what I'm talking about." http://www.olib.org/~rkr/ |