search

DIGImend / hidrd

HID report descriptor I/O library and conversion tool
GNU General Public License v2.0
171 stars 29 forks source link

Fix usage compare #9

Closed jigpu closed 8 years ago

jigpu commented 8 years ago

As outlined in the second patch, the comparison done by "hidrd_usage_valid" does not work properly if a usage greater than INT32_MAX is defined. This pull request fixes that, as well as updates "make check" (to validate the changes) since the "tempfile" command is long-since deprecated and not available on my system.

spbnick commented 8 years ago

Thanks a lot, Jason!

Unfortunately, I won't be able to merge this soon - I need to review this properly and try to think of a better way to handle it. However, I'm not likely to have time until two weeks from now - my Finnish course exams are coming up. If you need this in sooner, I guess I can merge it as it is, and then fix it up (if necessary). Otherwise I'd prefer to do a proper refresher of hidrd and review, maybe there's something else to fix as well.

BTW, did you notice if it's possible to add a test case for the enum range check?

jigpu commented 8 years ago

It doesn't impact me at all (just something I noticed when playing around with vendor defined usages), so take all the time you need. I hadn't looked into extending the test, but that's certainly a good idea if it can be done. I'm not terribly familiar with the codebase (it mostly Just Works :)), but I can try to help with ideas or review if you'd like.

Best wishes on those exams!

spbnick commented 8 years ago

Thanks, Jason :) Expect a proper response in two weeks.

spbnick commented 8 years ago

Hi Jason,

Sorry for the delay, still trying to deal with a lot of work after return to normal life after courses. I looked at the issue more closely and believe the original problem was that enum members higher than INT32_MAX were assumed to be negative as the literals weren't marked as unsigned and composed by bitshifts.

I made another pull request (#12), which marks full usage values as unsigned, so that doesn't happen, plus I switched the MIN/MAX macros to use the actual enum members, so there can't be a signedness mismatch anymore.

Could you please check if it will be fine to replace your pull request with that one?

Thank you.

jigpu commented 8 years ago

12 looks like a better solution, so feel free to drop this in favor of that. Only thing I'd ask is that 4ba834a ("Replace use of 'tempfile' with 'mktemp' in tests") from this request be merged or cherry-picked in addition to your own changes since I can't run make check on my system without it ;)

spbnick commented 8 years ago

Ah, yes, forgot about the mktemp. Sure, will pick it. Thanks for the submissions and the review!