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

[Rémi Bernon]

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.

Cheers
-- 
Rémi Bernon <rbernon at codeweavers.com>