[v2 1/1] dinput: Sort virtual joysticks by name on the Mac

[Ken Thomases]

On Jul 18, 2016, at 1:57 AM, David Lawrie <david.dljunk at gmail.com> wrote:

> +static CFStringRef copy_device_name(IOHIDDeviceRef device)
> +{
> +    CFTypeRef ref;
> +    CFStringRef name = NULL;
> +
> +    if(device)

Maybe didn't notice this in the previous version, but put a space between "if" and the parenthesis.

> +    {
> +        assert(IOHIDDeviceGetTypeID() == CFGetTypeID(device));
> +
> +        ref = IOHIDDeviceGetProperty(device, CFSTR(kIOHIDProductKey));
> +
> +        if (ref && CFStringGetTypeID() == CFGetTypeID(ref))
> +            name = CFStringCreateCopy(kCFAllocatorDefault,ref);

Please use a space after the comma in argument lists in function calls.

> +        else if (ref)
> +            name = CFCopyDescription(ref);
> +        else
> +            name = CFStringCreateCopy(kCFAllocatorDefault,CFSTR("(null)"));

You don't want to fall back to a string based on the vendor and product ID?

Also, the same spacing issue as above.

> +    }
> +    else
> +        ERR("Invalid Device requested %p\n",device);

At this point, device is known to be NULL, so the log message can say that rather than the vague "invalid", and doesn't need to include its value.  Or did you mean to be checking something else in the "if"?

> +
> +    return name;
> +}
> +
> static long get_device_location_ID(IOHIDDeviceRef device)
> {
>     return get_device_property_long(device, CFSTR(kIOHIDLocationIDKey));
> @@ -203,7 +227,18 @@ static void copy_set_to_array(const void *value, void *context)
>     CFArrayAppendValue(context, value);
> }
> 
> -static CFComparisonResult device_location_comparator(const void *val1, const void *val2, void *context)
> +static CFComparisonResult device_name_comparator(IOHIDDeviceRef device1, IOHIDDeviceRef device2)
> +{
> +    CFStringRef name1 = copy_device_name(device1), name2 = copy_device_name(device2);
> +    CFComparisonResult result = CFStringCompare(name1, name2, (kCFCompareForcedOrdering | kCFCompareNumerically));
> +    if(name1)
> +        CFRelease(name1);
> +    if(name2)
> +        CFRelease(name2);

If name1 or name2 might be NULL as these tests imply, then what does the preceding CFStringCompare() call do?  Probably crashes.  So, either assume/assure that copy_device_name() doesn't return NULL in normal operation or handle the possibility that it might more thoroughly.

-Ken