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

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/16/2010 01:12 PM, Brian Paul wrote:
> 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.

After seeing Jose's e-mail, I had to look this up to convince myself
that this was even correct behavior for the compiler.  It turns out that
it's not only correct, but it's required.  I guess you learn something
new everyday.  The F suffixes are in.

>>>         }
>>>         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.

Won't that generate the same warning as below?  How about if we just do
the following everywhere we want to get a bool from a numeric type:

	data.b[c] = (op[0]->value.f[c] != 0.0F);

I compared all three versions with GCC, and the exact same sequence of
code is generated for all each.

Since we actually want a bool result, this waring is completely
spurious.  I'd be most happy to disable the warning, but I can
compromise on using (x != 0).

>>>         }
>>>         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)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzkOrEACgkQX1gOwKyEAw9PNwCgkgSkW2rISPAOYEIioYzRkasV
ukUAnie3wuX96DQVr1XSkxIdk+o5V5Ek
=xW3S
-----END PGP SIGNATURE-----