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

[Alexei Fedorov <Alexei.Fedorov@arm.com>]

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?


Alexei.



________________________________
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.