From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-x232.google.com (mail-lf0-x232.google.com [IPv6:2a00:1450:4010:c07::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C3E558211F for ; Fri, 17 Feb 2017 04:30:15 -0800 (PST) Received: by mail-lf0-x232.google.com with SMTP id x1so21891313lff.0 for ; Fri, 17 Feb 2017 04:30:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=gpurnp5nzXsZr66OIDb4XU+o1G7w7LLSv+2SermXQ+E=; b=PA/mFYdsLd1yG7BMmY1nJh9T92ebK/ej3iw1I5q2RCR5JYCAo1P2GEDIMfWrDzbIe2 MtF/+XjN5eYnDLgCqjkrroy05f9XPOh4FaMmL/gmAYAIRsshAzQBPY3UTpwHn1qHkf4c G3oVIYCXoUFsFD3I7Lf5SU1csEalWJHkGo9TM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=gpurnp5nzXsZr66OIDb4XU+o1G7w7LLSv+2SermXQ+E=; b=l1Gx+9Ee8uAVj7eOJSM8mQ72KAuhaFzSfiyICZAwOYzptmvHu9eYacfRrbL7ZKrh6Z NJKQl/wo5VtxLGQgdZOo5Ra0EcerlPqANOxGOLce2p6OK6bsbg3Q3o9ibhc+p0FAWNAA JaSwPPNArft3gLMJd7SF9sEfPqPIQWwckraIQxEMmkCi4XqWInWdQp+xSO2UnShQ9kSI cfDXKf9pAXifMW5ThI8r8/ZRN3Zo76+kvDtx6gJXyVARly+VqqLdq5AZB5nCvcgTrwvw M8FKJ093eaZHJWmRHYHBlfhTKs7Pc/tWcHK8FPDfe57bW1AkovFvOP6iBp+c54meQx/x e4Lw== X-Gm-Message-State: AMke39l3Phog/pEGVYwsasTUnecHnDhQCayEqLQbBRQXZ7bwNuktxP9ajX+PK4BhTtAo9B/pHZtLQg45bVfc15gJ X-Received: by 10.46.80.71 with SMTP id v7mr2204719ljd.38.1487334613606; Fri, 17 Feb 2017 04:30:13 -0800 (PST) MIME-Version: 1.0 Received: by 10.25.207.72 with HTTP; Fri, 17 Feb 2017 04:30:13 -0800 (PST) In-Reply-To: References: <20170209192623.262044-1-evan.lloyd@arm.com> <20170209192623.262044-5-evan.lloyd@arm.com> <20170213121552.GU16034@bivouac.eciton.net> From: Ryan Harkin Date: Fri, 17 Feb 2017 12:30:13 +0000 Message-ID: To: Evan Lloyd Cc: "ard.biesheuvel@linaro.org" , Leif Lindholm , "edk2-devel@ml01.01.org" Subject: Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Feb 2017 12:30:16 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 17 February 2017 at 12:06, Evan Lloyd wrote: > Hi Ryan. > > > > From: Ryan Harkin [mailto:ryan.harkin@linaro.org] > Sent: 16 February 2017 20:42 > To: Evan Lloyd > Cc: ard.biesheuvel@linaro.org; Leif Lindholm; edk2-devel@ml01.01.org > Subject: RE: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType > functions > > > > > On 16 Feb 2017 20:27, "Evan Lloyd" wrote: >> >> Hi Leif. >> We accept all the comments except that about ternaries. >> Response inline. >> > =E2=80=A6 > >> >> + >> >> + *TriggerType =3D (MmioBitFieldRead32 (RegAddress, BitNumber, >> >BitNumber) =3D=3D 0) >> >> + ? EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH >> >> + : EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING; >> >> + >> > >> >Ternaries are excellent when they increase code readability. >> >I am not convinced that is the case here. >> > >> >Consider: >> > >> > if (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) =3D=3D 0) { >> > *TriggerType =3D EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH; >> > } else { >> > *TriggerType =3D EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING; >> > } >> > >> >? >> > >> >The versions generate identical code with gcc at -O1 and above. >> >> Firstly, I'm not sure why 5 lines of code is more readable than 3. >> My main point though is that the ternary expression clearly indicates >> that all we are doing is setting *TriggerType. >> The multiple assignment "if" requires examination to determine that ther= e >> is nothing else going on. (Because otherwise why wouldn't you use a >> ternary?) >> I'm about to submit v2 without this, in the hope that I've made the case= . >> > > I find your argument unconvincing and would prefer an "if" clause. > > That is fine, lots of people have irrational prejudices. ;-) > > Do you have a counter argument though? > I don't think I need a 3rd argument. Like Leif, I don't think that ternary adds clarity and think an '"if" statement would be easier to decipher. > Leif points out that =E2=80=9CTernaries are excellent when they increase = code > readability.=E2=80=9D > > The debate would thus seem to be a subjective assessment of =E2=80=9Cread= ability=E2=80=9D. > Indeed it is. > What criteria should we use to identify when a ternary is more readable, = and > when not? > > And how do we ensure consistency across all EDK2 maintainers? > None of that is down to me. Cheers, Ryan. > Evan > > > > =E2=80=A6 > > > > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy th= e > information in any medium. Thank you.