[Mesa-dev] Mesa (master): glsl: fix assorted MSVC warnings

[Brian Paul]

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