[PATCH 7/8] winexinput.sys: Return native product strings on some devices.

[Zebediah Figura]

On 9/6/21 4:08 PM, Rémi Bernon wrote:
> On 9/6/21 10:52 PM, Zebediah Figura wrote:
>> On 9/6/21 1:34 PM, Rémi Bernon wrote:
>>> On 9/6/21 7:06 PM, Zebediah Figura wrote:
>>>> On 9/6/21 1:40 AM, Rémi Bernon wrote:
>>>>> +    case IOCTL_HID_GET_STRING:
>>>>> +        switch
>>>>> ((ULONG_PTR)stack->Parameters.DeviceIoControl.Type3InputBuffer)
>>>>> +        {
>>>>> +        case HID_STRING_ID_IPRODUCT:
>>>>> +            match_id = wcsrchr(impl->device_id, '\\') + 1;
>>>>> +            for (i = 0; i < ARRAY_SIZE(device_strings); ++i)
>>>>> +                if (!wcsicmp(device_strings[i].id, match_id))
>>>>> +                    break;
>>>>> +            if (i < ARRAY_SIZE(device_strings)) str =
>>>>> device_strings[i].product;
>>>>> +            break;
>>>>
>>>> Just a suggestion, really, but this might be cleaner as a helper
>>>> function?
>>>>
>>> Well, I don't plan on adding anything more there, so maybe it could be
>>> split later if we need to.
>>
>> I don't necessarily mean anything needs to be added, it's just that I
>> really don't like the "for followed by if" pattern that this exemplifies.
>>
> 
> Well, it's probably a matter of taste. I find this pattern
> 
>     for (it; it != end; ++it) { if (matching) break }
>     if (it != end) { do something with it }
> 
> cleaner and more readable than the alternative that's often used:
> 
>     for (it; it != end; ++it)
>     {
>       if (matching)
>       {
>         do something with it
>         break
>       }
>     }
> 
> especially when "do something with it" is long: the break may be easily
> missed.
> 
> Of course, a "find_xxx" helper could make it even clearer, but I think
> the former pattern can be read as an implicit "find xxx" pattern,
> especially for small loops.

Right, the helper was what I was advocating for.

It's probably not worth arguing about, though.