Re: [Qemu-devel] [PATCH] ui/cocoa.m: Replace pc/xt keyboard keycode array with QKeyCode

[Eric Blake]

On 03/01/2016 05:12 PM, Programmingkid wrote:

>>
>>> +
>>> +    [MAC_KEY_ESC] = Q_KEY_CODE_ESC,
>>> +    //[MAC_KEY_F1] = Q_KEY_CODE_POWER, // Just in case you need the power 
>>> key
>>> +    [MAC_KEY_F1] = Q_KEY_CODE_F1,
>>
>> The comment looks weird. Probably worth a mention in the commit message
>> why you need it.
> 
> Maybe this:
> 
> [MAC_KEY_ESC] = Q_KEY_CODE_ESC,
> //[MAC_KEY_F1] = Q_KEY_CODE_POWER, // You might need this key if you use 
> Macsbug. 
> [MAC_KEY_F1] = Q_KEY_CODE_F1,

Not a code comment, but a commit message comment stating why you aren't
providing a mapping for q_KEY_CODE_POWER.  For that matter, I don't
think a code comment is useful, just nuke the entire line (the comment
doesn't add anything).

> 
> 
>>
>>>
>>> static int cocoa_keycode_to_qemu(int keycode)
>>> {
>>> -    if (ARRAY_SIZE(keymap) <= keycode) {
>>> +    if (ARRAY_SIZE(macToQKeyCodeMap) <= keycode) {
>>>         fprintf(stderr, "(cocoa) warning unknown keycode 0x%x\n", keycode);
>>
>> Pre-existing, but we should fix this to avoid fprintf.
> 
> What do you mean by pre-existing?

You weren't the original cause of the bug, so it is not necessarily this
patch's job to fix the bug.  Therefore, "pre-existing".  But since the
bug was observed during review of your patch, you may want to fix it
anyways, probably as a separate patch.

> I personally don't have anything against fprintf, but switching to printf is 
> just fine with me.

No, don't switch to printf.  The bug is that we DON'T want to use printf
or fprintf for error reporting.  Rather, you should do proper error
reporting, such as populating an Error **errp parameter, if the error
needs reporting at all; or else delete the line if the code continues
just fine in spite of the unknown keycode.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature