From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58573) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaubc-0000Sx-4g for qemu-devel@nongnu.org; Tue, 01 Mar 2016 19:25:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aaubY-0007J5-V3 for qemu-devel@nongnu.org; Tue, 01 Mar 2016 19:25:32 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42267) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaubY-0007Il-Na for qemu-devel@nongnu.org; Tue, 01 Mar 2016 19:25:28 -0500 References: <6FDF78F9-D147-4239-9B8E-CAB1417369AA@gmail.com> <56D622BC.6090600@redhat.com> <296156E0-31D8-4612-B3B7-AB93093353D3@gmail.com> From: Eric Blake Message-ID: <56D632F7.7000500@redhat.com> Date: Tue, 1 Mar 2016 17:25:27 -0700 MIME-Version: 1.0 In-Reply-To: <296156E0-31D8-4612-B3B7-AB93093353D3@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="5pdeke9VX9A81exGHlVoTQGcDx8kJHjXs" Subject: Re: [Qemu-devel] [PATCH] ui/cocoa.m: Replace pc/xt keyboard keycode array with QKeyCode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Programmingkid Cc: Peter Maydell , qemu-devel qemu-devel This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --5pdeke9VX9A81exGHlVoTQGcDx8kJHjXs Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 03/01/2016 05:12 PM, Programmingkid wrote: >> >>> + >>> + [MAC_KEY_ESC] =3D Q_KEY_CODE_ESC, >>> + //[MAC_KEY_F1] =3D Q_KEY_CODE_POWER, // Just in case you need th= e power key >>> + [MAC_KEY_F1] =3D Q_KEY_CODE_F1, >> >> The comment looks weird. Probably worth a mention in the commit messag= e >> why you need it. >=20 > Maybe this: >=20 > [MAC_KEY_ESC] =3D Q_KEY_CODE_ESC, > //[MAC_KEY_F1] =3D Q_KEY_CODE_POWER, // You might need this key if you = use Macsbug.=20 > [MAC_KEY_F1] =3D 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). >=20 >=20 >> >>> >>> static int cocoa_keycode_to_qemu(int keycode) >>> { >>> - if (ARRAY_SIZE(keymap) <=3D keycode) { >>> + if (ARRAY_SIZE(macToQKeyCodeMap) <=3D keycode) { >>> fprintf(stderr, "(cocoa) warning unknown keycode 0x%x\n", key= code); >> >> Pre-existing, but we should fix this to avoid fprintf. >=20 > 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 prin= tf 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --5pdeke9VX9A81exGHlVoTQGcDx8kJHjXs Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJW1jL3AAoJEKeha0olJ0NqoDIH/195e2FV1C6xcziGTUqALptH WmEZurBUQ0u2vAU/LtOXs98Geqlh0V23SVazE1+vM/WhF86NTUld2HCZqFKzREli GsqZNedQetMufC5VEM31tI72+WRIN0+mpklKi6hV8xP4i37lvyElsWDDBR2yB9UP NTc7zhVdkix1uJEO2dZ7Pq628U+h7WehPABLOf0AJSHm4gyKode0eqP1h+sU08Hm UCnbLMOY9I0XWtSN+ED8xN4RY4Ce2XrLpKBHTVDmB/ZCknaiaBHCWuFEi5SpVix/ drcqzDuNObxR0P1c/l9gh7H9HKFy0XtGwIKrevWqKfLSvVtgbd8fVmbS7H/wVAM= =s1ZF -----END PGP SIGNATURE----- --5pdeke9VX9A81exGHlVoTQGcDx8kJHjXs--