On 11/16/2010 12:55 PM, Ian Romanick wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/15/2010 05:48 PM, Brian Paul wrote:

     case ir_unop_b2f:
        assert(op[0]->type->base_type == GLSL_TYPE_BOOL);
        for (unsigned c = 0; c<  op[0]->type->components(); c++) {
-        data.f[c] = op[0]->value.b[c] ? 1.0 : 0.0;
+        data.f[c] = op[0]->value.b[c] ? 1.0F : 0.0F;

Please don't do this.  This particular MSVC warning should just be
disabled.  If this warning were generated for non-literals and for
literals that actually did lose precision being stored to a float, it
might have a chance at having some value.  Instead, it's just noise.

Individual warnings can be disabled with a pragma, and this one should
probably be disabled in mesa/compiler.h:

#pragma warning(disable: 4244)

There may be a way to do it from the command line, but I don't know what
it is.

The F suffixes on constants are also worthless, and they make the code
ugly.  Expecting that they will be added everywhere when no other
compiler generates this warning is a losing battle.

I've been in the habit of using F suffixes for many, many years. Back in my IRIX days it told the compiler to use a float and not a double for the computation (which was faster). And according to Jose's email which I just spotted, that's still the case with gcc.

I recall another another old unix compiler that I used back then (maybe AIX) issued warnings similar to MSVC. Old habits die hard. But I don't think it's a bad habit.



        }
        break;
     case ir_unop_f2b:
        assert(op[0]->type->base_type == GLSL_TYPE_FLOAT);
        for (unsigned c = 0; c<  op[0]->type->components(); c++) {
-        data.b[c] = bool(op[0]->value.f[c]);
+        data.b[c] = op[0]->value.f[c] != 0.0F ? true : false;

This warning should also be disabled for the same reason as the above.
This one isn't even a correctness warning, is a performance warning.
The code that is replacing the case may have even worse performance than
the cast!  The other changes can stay, but this one needs to be reverted.

Perhaps data.b[c] = bool((int) op[0]->value.f[c]);  would do the trick.


        }
        break;
     case ir_unop_b2i:
@@ -163,7 +163,7 @@ ir_expression::constant_expression_value()
     case ir_unop_i2b:
        assert(op[0]->type->is_integer());
        for (unsigned c = 0; c<  op[0]->type->components(); c++) {
-        data.b[c] = bool(op[0]->value.u[c]);
+        data.b[c] = op[0]->value.u[c] ? true : false;

What warning is this?  I was unable to reproduce a warning on Visual
Studio 2008 Express Edition.  I suspect this should be reverted too.

The warning was:

src\glsl\ir_constant_expression.cpp(166) : warning C4800: 'unsigned int' : forcing value to bool 'true' or 'false' (performance warning)

-Brian
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to