From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22d.google.com (mail-it0-x22d.google.com [IPv6:2607:f8b0:4001:c0b::22d]) (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 AA0F981F06 for ; Fri, 17 Feb 2017 10:18:17 -0800 (PST) Received: by mail-it0-x22d.google.com with SMTP id g67so23778202itb.1 for ; Fri, 17 Feb 2017 10:18:17 -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=WyskSc/xMSujWgHsFjKN0Rab061M9cw1FAKZPmaFkH8=; b=c3CRPa7+9LmMyxySXcnZySw/fkCo8Yqnp0OUQ/eTNe0z/13rczV4/KXiRCrs2BZ6gk REQYRR391C+DoqHs44gLnpyIdvqBb2iQkgN/lqnkUmYhjASXA1LoNNeXIb+cAF99np+S j/l6MSWKd8v4u9bGgegCt8eCnHWpXpHpkdUOE= 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=WyskSc/xMSujWgHsFjKN0Rab061M9cw1FAKZPmaFkH8=; b=LSc9Y2s+MsGcnRySBbe6HU0h3Yl6yrOWntn2KTXNSA+15oh+LuU5kGzbLYFCBrwdvS vDXAfP+FcSBLb9wak2B+nAoVZvWFENH8OgUVwnfGE7vDUup8HnSg7ei8ySeD9k8Cd5li afTbG1fkdjzhghQHn68AEiYltYHCBdiPUKzebhoXTnfq03mpk/1kzj3jl86iUuu49Yd6 +TFouE0UYu5DcXl9LruCMHN3COimlFfDofeJdVJWQjva6zEJ5Wzpf4F1v2kbVXQVR03z c9RylOAkGBXkzwslA5kCiV/eXqjX4IQgLT2+FPYGnPGWDZHYhGsxGn+ztN4DSZzi7b3F WIXw== X-Gm-Message-State: AMke39mG8+rYauc6VqOBLDiv6OxJpxgwhbH4Z/YphCMt7LMIka859OKPoytEd9hK/6g8KiEoi+leB/RxN6+z/1Oq X-Received: by 10.107.13.130 with SMTP id 124mr18677ion.83.1487355497013; Fri, 17 Feb 2017 10:18:17 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.198.134 with HTTP; Fri, 17 Feb 2017 10:18:16 -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: Ard Biesheuvel Date: Fri, 17 Feb 2017 18:18:16 +0000 Message-ID: To: Alexei Fedorov Cc: "ryan.harkin@linaro.org" , Evan Lloyd , "edk2-devel@ml01.01.org" , Leif Lindholm 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 18:18:17 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 17 February 2017 at 15:08, Alexei Fedorov wrote= : > Please take a look at the following code in GenericWatchdogEntry(): > > > Status =3D mInterruptProtocol->RegisterInterruptSource ( > mInterruptProtocol, > FixedPcdGet32 > (PcdGenericWatchdogEl2IntrNum), > WatchdogInterruptHandler > ); > if (!EFI_ERROR (Status)) { > Status =3D mInterruptProtocol->SetTriggerType ( > mInterruptProtocol, > FixedPcdGet32 > (PcdGenericWatchdogEl2IntrNum), > > EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING); > > > Why can't we extend EFI_HARDWARE_INTERRUPT2_PROTOCOL > RegisterInterruptSource() function API to have an extra > EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType parameter to set interr= upt > type & get rid of the second call? > We could do that, yes. I tried to keep the members shared between the old and the new version of the protocol identical, so that the latter could potentially be cast to the former. I did not investigate extensively whether there is any point to doing that, though. it just seemed like a sensible thing to do for two-way compatibility > ________________________________ > From: edk2-devel on behalf of Ryan Hark= in > > Sent: 17 February 2017 12:30:13 > To: Evan Lloyd > Cc: edk2-devel@ml01.01.org; Leif Lindholm; ard.biesheuvel@linaro.org > Subject: Re: [edk2] [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerT= ype > functions > > 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 the= re >>> 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 cas= e. >>> >> >> 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=9Crea= dability=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 t= he >> information in any medium. Thank you. > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > 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.