If I may reply in some detail ...

(1) Yes.  The "||" should be a "&&".

(2) I agree.  I also would like to take out the "!= NULL".

(3) I have no objection to rewriting the code on stylistic grounds.  If it were stable and bugfree, there would be some grounds for not fiddling with it, but as it is it needs all the help it can get.

John F. Fay
john.fay@eglin.af.mil



-----Original Message-----
From: Richard Rauch [mailto:sforge@olib.org]
Sent: Monday, October 27, 2003 12:20 AM
To: freeglut-developer@lists.sourceforge.net
Subject: [Freeglut-developer] Menu bug?


[If you don't see the point of this email, skip to the last couple of
 paragraphs...]

Shouldn't the || in this be a &&?

    /*
     * There must be a current window and a current menu set:
     */
    freeglut_return_if_fail( fgStructure.Window != NULL || fgStructure.Menu != NULL );

..?

I think so.  If fgStructure is non-NULL, then the value of fgStructure.Menu does not
matter, and the following code will execute---some of which attempts to use
fgStructure.Menu.


As a point of style, I would like to suggest that:

  x != NULL

...is harder to read than:

  x

...as a test for x's validity.  I know that I have to think appreciably harder
about the former than the latter.  I believe that the existing code is a bug, and
that the bug exists precisely because the "x != NULL" is more work to parse (or
to construct), mentally.  (Well, it's also complicated because of the odd
"return if fail", which is semantically a negation.  What the author
presumably wanted was "If this fails or if that fails", but instead they got
"if this-OR-that fails" -=> "if this-fails AND that-fails".)


The menu code's bug could be fixed by simply changing || to &&.  We could
also get rid of the {!= NULL}s.  Even better would be to write it as two
freeglut_return_if_fail() operations:

    freeglut_return_if_fail( fgStructure.Window );
    freeglut_return_if_fail( fgStructure.Menu );

IMHO, this is far clearer.

I raise this general point because there's lots of code that uses the
(in my eyes) obfusticating "if( x != NULL )" construct when "if( x )" is
so much simpler and clearer.  This is a style issue which, if addressed,
will improve the code's clarity greatly at certain points.

Are there any objections to changing this (as a general style improvement
to the code, not just fixing the one local bug), or is the former actually
preferred for some reason?


--
  "I probably don't know what I'm talking about."  http://www.olib.org/~rkr/


-------------------------------------------------------
This SF.net email is sponsored by: The SF.net Donation Program.
Do you like what SourceForge.net is doing for the Open
Source Community?  Make a contribution, and help us add new
features and functionality. Click here: http://sourceforge.net/donate/
_______________________________________________
Freeglut-developer mailing list
Freeglut-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/freeglut-developer