Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions

[Ryan Harkin <ryan.harkin@linaro.org>]

On 17 February 2017 at 12:06, Evan Lloyd <Evan.Lloyd@arm.com> 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" <Evan.Lloyd@arm.com> wrote:
>>
>> Hi Leif.
>> We accept all the comments except that about ternaries.
>> Response inline.
>>
> …
>
>> >> +
>> >> +  *TriggerType = (MmioBitFieldRead32 (RegAddress, BitNumber,
>> >BitNumber) == 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) == 0) {
>> >    *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
>> >  } else {
>> >    *TriggerType = 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 there
>> 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 “Ternaries are excellent when they increase code
> readability.”
>
> The debate would thus seem to be a subjective assessment of “readability”.
>

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
>
>
>
> …
>
>
>
> 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 the
> information in any medium. Thank you.