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. Here, the right solution is probably to use a signed counter, and explicitly cast uint -> int to shut up the compiler. --Ken
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
