From: Eric Blake <eblake@redhat.com>
To: Programmingkid <programmingkidx@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
qemu-devel qemu-devel <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH] ui/cocoa.m: Replace pc/xt keyboard keycode array with QKeyCode
Date: Tue, 1 Mar 2016 17:25:27 -0700 [thread overview]
Message-ID: <56D632F7.7000500@redhat.com> (raw)
In-Reply-To: <296156E0-31D8-4612-B3B7-AB93093353D3@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1911 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
next prev parent reply other threads:[~2016-03-02 0:25 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-01 22:12 [Qemu-devel] [PATCH] ui/cocoa.m: Replace pc/xt keyboard keycode array with QKeyCode Programmingkid
2016-03-01 23:16 ` Eric Blake
2016-03-02 0:12 ` Programmingkid
2016-03-02 0:25 ` Eric Blake [this message]
2016-03-02 1:20 ` Programmingkid
2016-03-02 2:14 ` Eric Blake
2016-03-02 9:29 ` Markus Armbruster
2016-03-01 23:18 ` Peter Maydell
2016-03-02 0:18 ` Programmingkid
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56D632F7.7000500@redhat.com \
--to=eblake@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=programmingkidx@gmail.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).