Re: More fallout from TBOOLEAN change - hidden3d broken

[Daniel J S. <dan...@ie...>]

Ethan Merritt wrote:

>Is there a tool (or gcc compile option) that will
>double check for illegal operations on our TBOOLEANs? 
>
>In stepping through the demos to check 3.8k.3 I found
>that something very ugly has happened to hidden3d.
>See "surface2.dem".
>
>Reverting to the previous version of src/syscfg.h
>made the problem go away, so this was another
>bit of fallout from the TBOOLEAN change.
>
>hidden3d.c line 817:
>        casenumber = (plist[new_poly].frontfacing != 0)
>            + 2 * (plist[old_poly].frontfacing != 0);
>
>frontfacing is a TBOOLEAN.
>
>Even worse:
>            TBOOLEAN v1_inside_p, v2_inside_p;
>             unsigned int classification[]
>
>-- 
>            v1_inside_p = ~ (classification[0]
>                             | classification[1]
>                             | classification[2]);
>--
>            v2_inside_p = (v1_inside_p >> 1) & 0x1;
>            v1_inside_p = v1_inside_p & 0x1;
>
>I have corrected the syntax involving frontfacing,
>and changed v?_inside_p to be (unsigned int).
>This fixes surface2.dem, but other problems may lurk.
>

That fixes things and is fine.  However, v1_inside_p and v2_inside_p do 
have the feel of Boolean variables.  The problem is that v1_inside_p is 
used as a temp variable and from a clean C programming viewpoint 
probably shouldn't be.  Also, doing the bit shift on the variable is 
extraneous.  Could have something like:


#define V1_INSIDE_P_BIT 1
#define V2_INSIDE_P_BIT 2

unsigned_int triangle_classification;

        triangle_classification = ~ (classification[0]
                 | classification[1]
                 | classification[2]);

        v1_inside_p = (traingle_classification & V1_INSIDE_P_BIT);
        v2_inside_p = (triangle_classification & V2_INSIDE_P_BIT);

But, if one goes a step further in the code at that point, you'll notice 
that the logic is

        if ((v1_inside_p) && (v2_inside_p)) {

and "v1_inside_p" and "v2_inside_p" aren't used afterward.  So really, 
is it necessary to break this into two individual tests and recombine 
them?  (Future modification perhaps that I'm missing?)   I mean, in the 
definition of "classify()" there is specific use of "1" and "2" rather 
than definitions such as V1_INSIDE_P_BIT AND V2_INSIDE_P_BIT, so if you 
want to be loose about this subroutine and end up with efficient code 
rather than reader friendly code, one could just do:

        triangle_classification = ~ (classification[0]
                 | classification[1]
                 | classification[2]);

        if (triangle_classification >= 3) {

Dan

-- 

Dan Sebald
email: daniel DOT sebald AT ieee DOT org
URL: http://acer-access DOT com/~dsebald AT acer-access DOT com/