[PATCH] d3dx9: Don't check for negative enum value.

[Matteo Bruni]

On Tue, Mar 24, 2020 at 4:06 PM Paul Gofman <gofmanp at gmail.com> wrote:
>
> On 3/24/20 17:39, Matteo Bruni wrote:
> > On Tue, Mar 24, 2020 at 3:29 PM Jacek Caban <jacek at codeweavers.com> wrote:
> >> On 24.03.2020 14:59, Henri Verbeet wrote:
> >>> On Tue, 24 Mar 2020 at 17:45, Matteo Bruni <matteo.mystral at gmail.com> wrote:
> >>>> I assume replacing "op < 0" with an explicit "op < SCT_VSFLOAT"
> >>>> doesn't avoid the warning. Does replacing the if with an assert do the
> >>>> trick, by any chance?
> >>>>
> >>> It's perhaps a little subtle, but if you compare "op" against
> >>> "ARRAY_SIZE(const_tbl)" (which is what you really care about here
> >>> anyway, right?) instead of SCT_PSINT, you can drop the check against
> >>> 0, regardless of whether the enum ends up being a signed or unsigned
> >>> type.
> >>
> >> Strictly speaking, if enum would be signed the check wouldn't catch some
> >> invalid values, but I don't think that's worrying about. If we want it
> >> fully strict, we could change argument type from enum to unsigned int.
> >>
> >>
> >> Thanks,
> >>
> >> Jacek
> > I think sizeof() (and thus ARRAY_SIZE()) always returns an unsigned
> > integer, op is promoted to unsigned if necessary and the comparison is
> > guaranteed to be unsigned.
> >
> If op will be considered signed, won't it end up in the warning about
> signed vs unsigned comparison? Regardless of that, maybe we instead add
> a check to state->operation read from effect data in d3dx_parse_state()
> and just remove any of such checks at effect runtime?

Eh, we're working around an overzealous compiler warning (I'd argue
that the compiler has no business complaining for this in the first
place), whatever works in silencing the warning is fine IMHO. I take
Jacek's sign-off to mean that the warning with clang goes away. It
still doesn't cause any new warnings with gcc for me.
I do think that the assert with ARRAY_SIZE() is nicer than the
previous if and it's not a performance concern (you're supposed to
compile with -DNDEBUG if you really care about performance). But you
bring a good point in that we should also check that state->operation
doesn't overrun state_table[]. That one should be checked at parse
time and it should be a normal if () + WARN(), since it depends on the
effect data. Care to send a patch? :)