Re: [PATCH] hidclass.sys: fixed copy/paste error (Coverity)
On Oct 3, 2015, at 2:52 AM, Marcus Meissner <marcus(a)jet.franken.de> wrote:
1325460 Identical code for different branches
Signed-off-by: Marcus Meissner <marcus(a)jet.franken.de> --- dlls/hidclass.sys/descriptor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/hidclass.sys/descriptor.c b/dlls/hidclass.sys/descriptor.c index 6f8e45e..35057f1 100644 --- a/dlls/hidclass.sys/descriptor.c +++ b/dlls/hidclass.sys/descriptor.c @@ -414,7 +414,7 @@ void parse_io_feature(unsigned int bSize, int itemVal, int bTag, unsigned int *f if ((itemVal & INPUT_ARRAY) == 0) feature->isArray= TRUE; else - feature->isArray= TRUE; /* Var */ + feature->isArray= FALSE; /* Var */
Hmm. First, doesn't that logic seem backward? You're now setting isArray to true only if itemVal does NOT contain INPUT_ARRAY. And wouldn't it be better to just replace that whole if statement by: feature->isArray = (itemVal & INPUT_ARRAY) != 0; ? (For what it's worth, I started to suggest the style refactoring first and that's what made it so obvious that the logic was wrong. The power of good style to improve comprehension!)
if ((itemVal & INPUT_ABS) == 0) feature->IsAbsolute = TRUE; else
The rest of this function looks like it's riddled with similar backward logic. :( -Ken
On 10/3/15 4:01 AM, Ken Thomases wrote:
On Oct 3, 2015, at 2:52 AM, Marcus Meissner <marcus(a)jet.franken.de> wrote:
1325460 Identical code for different branches
Signed-off-by: Marcus Meissner <marcus(a)jet.franken.de> --- dlls/hidclass.sys/descriptor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/hidclass.sys/descriptor.c b/dlls/hidclass.sys/descriptor.c index 6f8e45e..35057f1 100644 --- a/dlls/hidclass.sys/descriptor.c +++ b/dlls/hidclass.sys/descriptor.c @@ -414,7 +414,7 @@ void parse_io_feature(unsigned int bSize, int itemVal, int bTag, unsigned int *f if ((itemVal & INPUT_ARRAY) == 0) feature->isArray= TRUE; else - feature->isArray= TRUE; /* Var */ + feature->isArray= FALSE; /* Var */
Hmm. First, doesn't that logic seem backward? You're now setting isArray to true only if itemVal does NOT contain INPUT_ARRAY.
How it works is that each bit is a named flag that has one meaning when on and another when off. I, admittedly confusingly, mostly named each bit after its off (0), flag meaning. Most of the example parsers I found tended to do it this way. It was less confusing when I was using raw hex values than when I named the flags. :) -aric
On Oct 4, 2015, at 12:26 PM, Aric Stewart <aric(a)codeweavers.com> wrote:
On 10/3/15 4:01 AM, Ken Thomases wrote:
On Oct 3, 2015, at 2:52 AM, Marcus Meissner <marcus(a)jet.franken.de> wrote:
diff --git a/dlls/hidclass.sys/descriptor.c b/dlls/hidclass.sys/descriptor.c index 6f8e45e..35057f1 100644 --- a/dlls/hidclass.sys/descriptor.c +++ b/dlls/hidclass.sys/descriptor.c @@ -414,7 +414,7 @@ void parse_io_feature(unsigned int bSize, int itemVal, int bTag, unsigned int *f if ((itemVal & INPUT_ARRAY) == 0) feature->isArray= TRUE; else - feature->isArray= TRUE; /* Var */ + feature->isArray= FALSE; /* Var */
Hmm. First, doesn't that logic seem backward? You're now setting isArray to true only if itemVal does NOT contain INPUT_ARRAY.
How it works is that each bit is a named flag that has one meaning when on and another when off. I, admittedly confusingly, mostly named each bit after its off (0), flag meaning.
May I humbly suggest that you consider renaming those to make the code less horrible? -Ken
On 10/4/15 1:03 PM, Ken Thomases wrote:
On Oct 4, 2015, at 12:26 PM, Aric Stewart <aric(a)codeweavers.com> wrote:
On 10/3/15 4:01 AM, Ken Thomases wrote:
On Oct 3, 2015, at 2:52 AM, Marcus Meissner <marcus(a)jet.franken.de> wrote:
diff --git a/dlls/hidclass.sys/descriptor.c b/dlls/hidclass.sys/descriptor.c index 6f8e45e..35057f1 100644 --- a/dlls/hidclass.sys/descriptor.c +++ b/dlls/hidclass.sys/descriptor.c @@ -414,7 +414,7 @@ void parse_io_feature(unsigned int bSize, int itemVal, int bTag, unsigned int *f if ((itemVal & INPUT_ARRAY) == 0) feature->isArray= TRUE; else - feature->isArray= TRUE; /* Var */ + feature->isArray= FALSE; /* Var */
Hmm. First, doesn't that logic seem backward? You're now setting isArray to true only if itemVal does NOT contain INPUT_ARRAY.
How it works is that each bit is a named flag that has one meaning when on and another when off. I, admittedly confusingly, mostly named each bit after its off (0), flag meaning.
May I humbly suggest that you consider renaming those to make the code less horrible?
Sounds good. I will likely be unable to get to it immediately but will try to as soon as I can. -aric
participants (2)
-
Aric Stewart -
Ken Thomases