[PATCH] d3dx9: Don't check for negative enum value.
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- Fixes clang warning: ../../../dlls/d3dx9_37/../d3dx9_36/effect.c:1472:12: warning: result of comparison of unsigned enum expression < 0 is always false [-Wtautological-unsigned-enum-zero-compare] dlls/d3dx9_36/effect.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Don't some compilers pick 'int' instead of 'unsigned int' for enums? Chip
Hi Chip, I believe the type depends on the default values the enum variables are assigned. As long as the values fit within that type, its acceptable. A default value of -1 would need to be signed for example. Alistair. On 24/3/20 11:53 am, Chip Davis wrote:
Don't some compilers pick 'int' instead of 'unsigned int' for enums?
Chip
On 24.03.2020 01:53, Chip Davis wrote:
Don't some compilers pick 'int' instead of 'unsigned int' for enums?
Compiler has some freedom to do that, yes, but in this case we always pass values from an internal table and we know that it's never negative. In fact, I'm not sure why the check is there in the first place. It could be an assert() or just skipped, unless I'm missing something. Thanks, Jacek
On Tue, Mar 24, 2020 at 2:23 AM Jacek Caban <jacek(a)codeweavers.com> wrote:
On 24.03.2020 01:53, Chip Davis wrote:
Don't some compilers pick 'int' instead of 'unsigned int' for enums?
Compiler has some freedom to do that, yes, but in this case we always pass values from an internal table and we know that it's never negative. In fact, I'm not sure why the check is there in the first place. It could be an assert() or just skipped, unless I'm missing something.
Thanks,
Jacek
Well, that table is used for different states and, in particular, the relevant field is "overloaded" to store enum SHADER_CONSTANT_TYPE values only for the shader constant entries. Leaving a more serious rework of the whole thing aside for the moment, it would be nice to keep some sort of validation for the table. 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?
On Tue, 24 Mar 2020 at 17:45, Matteo Bruni <matteo.mystral(a)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.
On Tue, Mar 24, 2020 at 2:59 PM Henri Verbeet <hverbeet(a)gmail.com> wrote:
On Tue, 24 Mar 2020 at 17:45, Matteo Bruni <matteo.mystral(a)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.
Oh, I like that! Yes, that would definitely work. I'm going to type the patch unless anyone beats me to it.
On 24.03.2020 14:59, Henri Verbeet wrote:
On Tue, 24 Mar 2020 at 17:45, Matteo Bruni <matteo.mystral(a)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
On Tue, Mar 24, 2020 at 3:29 PM Jacek Caban <jacek(a)codeweavers.com> wrote:
On 24.03.2020 14:59, Henri Verbeet wrote:
On Tue, 24 Mar 2020 at 17:45, Matteo Bruni <matteo.mystral(a)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.
On 24.03.2020 15:39, Matteo Bruni wrote:
On Tue, Mar 24, 2020 at 3:29 PM Jacek Caban <jacek(a)codeweavers.com> wrote:
On 24.03.2020 14:59, Henri Verbeet wrote:
On Tue, 24 Mar 2020 at 17:45, Matteo Bruni <matteo.mystral(a)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.
Sure, but theoretically if underlying type of enum is signed char (actually, signed part is not important), then it will be truncated before being promoted. Jacek
On 3/24/20 17:39, Matteo Bruni wrote:
On Tue, Mar 24, 2020 at 3:29 PM Jacek Caban <jacek(a)codeweavers.com> wrote:
On 24.03.2020 14:59, Henri Verbeet wrote:
On Tue, 24 Mar 2020 at 17:45, Matteo Bruni <matteo.mystral(a)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?
On Tue, Mar 24, 2020 at 4:06 PM Paul Gofman <gofmanp(a)gmail.com> wrote:
On 3/24/20 17:39, Matteo Bruni wrote:
On Tue, Mar 24, 2020 at 3:29 PM Jacek Caban <jacek(a)codeweavers.com> wrote:
On 24.03.2020 14:59, Henri Verbeet wrote:
On Tue, 24 Mar 2020 at 17:45, Matteo Bruni <matteo.mystral(a)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? :)
On 3/24/20 18:29, Matteo Bruni wrote:
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? :)
Yes, sure. I guess besides WARN it would make sense to fail effect creation if we meet an unexpected state operation.
On Tue, Mar 24, 2020 at 4:54 PM Paul Gofman <gofmanp(a)gmail.com> wrote:
On 3/24/20 18:29, Matteo Bruni wrote:
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? :)
Yes, sure. I guess besides WARN it would make sense to fail effect creation if we meet an unexpected state operation.
Yep, agreed.
participants (6)
-
Alistair Leslie-Hughes -
Chip Davis -
Henri Verbeet -
Jacek Caban -
Matteo Bruni -
Paul Gofman