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

[Evan Lloyd <Evan.Lloyd@arm.com>]

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<mailto: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?

Leif points out that “Ternaries are excellent when they increase code readability.”

The debate would thus seem to be a subjective assessment of “readability”.

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?

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.