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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 17 February 2017 at 15:08, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
> Please take a look at the following code in GenericWatchdogEntry():
>
>
>   Status = mInterruptProtocol->RegisterInterruptSource (
>                                     mInterruptProtocol,
>                                     FixedPcdGet32
> (PcdGenericWatchdogEl2IntrNum),
>                                     WatchdogInterruptHandler
>                                     );
>       if (!EFI_ERROR (Status)) {
>         Status = 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 interrupt
> 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 <edk2-devel-bounces@lists.01.org> on behalf of Ryan Harkin
> <ryan.harkin@linaro.org>
> 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/SetTriggerType
> functions
>
> 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.
> _______________________________________________
> 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 the
> information in any medium. Thank you.