On 06/02/2014 05:49 PM, Kenneth Graunke wrote:
> On Monday, June 02, 2014 03:48:23 PM Ian Romanick wrote:
>> On 05/31/2014 10:11 PM, Kenneth Graunke wrote:
>>> On Tuesday, May 27, 2014 06:47:36 PM Matt Turner wrote:
>>> [snip]
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>>> b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>>>> index 069b60f..a1aff21 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>>>> @@ -353,7 +353,7 @@ fs_visitor::try_constant_propagate(fs_inst *inst,
>>> acp_entry *entry)
>>>> if (entry->src.file != IMM)
>>>> return false;
>>>>
>>>> - for (int i = 2; i >= 0; i--) {
>>>> + for (int i = inst->sources - 1; i >= 0; i--) {
>>>> if (inst->src[i].file != entry->dst.file ||
>>>> inst->src[i].reg != entry->dst.reg ||
>>>> inst->src[i].reg_offset != entry->dst.reg_offset ||
>>>
>>> I am glad to see that you're using a signed integer type here. For
>>> BRW_OPCODE_NOP, SHADER_OPCODE_PLACEHOLDER_HALT, and so on, inst->sources
> could
>>> conceievably be 0. (Today, you set it to 3 for some reason, but I could
> see
>>> someone coming along and "fixing" that later...)
>>>
>>> If you had used unsigned types, this would translate into i = 4294967296;
> i >=
>>> 0; i--, which would take forever. With signed types, it works, and no one
>>> should get bitten by that bug.
>>
>> Consistency of types is good. Some folks build with additional
>> warnings, and the flood of
>>
>> main/bufferobj.c:2855:18: warning: comparison between signed and unsigned
> integer expressions [-Wsign-compare]
>>
>> can hide legit bugs. :(
>
> I'm not advocating against leaving compiler warnings. I'm advocating against
> naively changing it to unsigned in order to "shut up" those warnings, and
> actually /introduce/ bugs.Of course! Me neither. :) > Here, the right solution is probably to use a signed counter, and explicitly > cast uint -> int to shut up the compiler. I guess the part I was thinking but didn't say is why not just use signed in all these places? I don't care too much either way... I just want less bugs and less warning noise. :) > --Ken
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
