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

[Leif Lindholm <leif.lindholm@linaro.org>]

(Intentionally sending my responses out of order)

Hi Evan,

On Fri, Feb 17, 2017 at 12:06:30PM +0000, Evan Lloyd wrote:
> 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. With the asymmetric weight distribution towards readability by
the maintainer.

> What criteria should we use to identify when a ternary is more
> readable, and when not?

My view of readability of ternaries is that it comes down to the
amount of information decoding required.

The Tianocore coding style actually works against ternaries a bit, in
that the resulting long variable/function/macro names frequently cause
them to spill onto multiple lines, which usually (but not always)
decrease readability.

The sequence in question
---
  *TriggerType = (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) == 0)
                 ?  EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH
                 :  EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
---
looks like it's extracting a bitfield and assigning it to
*TriggerType, until you get to the == 0, where it looks like it's
assigning whether or not the value of an extracted bitfield is equal
to 0, and you have to get to the next line before you even spot that
it's a ternary. That's twice I have to re-evaluate _what_ it is I am
reviewing before I get to actually reviewing it.

At which point it becomes clear that the "bitfield extraction" is
actually being used as a roundabout binary AND operation. Another
re-evaluation of the sequence.

Had it been written as
---
  *TriggerType = (MmioRead32 (RegAddress) & (1U << BitNumber) == 0)
---
it wouldn't have slowed me down as much, and I might not even have
objected, only grumbled a bit under my breath.

The if-form, while 2 lines longer, contains the two lines
  } else {
and
  }
, which are completely trivial. So while two lines are added, they do
not add two lines to review. And it makes it completely clear from the
get-go that what I am reviewing is a test assigning one value or
another depending on the result of the test.

It also means (although it too would be improved by replacing the
BitField form with the binary AND) it at least isolates the test
statement.

Examples of ternary use that I approve of:
>From Linux kernel:
  arch/arm64/mm/dump.c
  .name   = (CONFIG_PGTABLE_LEVELS > 3) ? "PUD" : "PGD",

>From EDK2:
  MdeModulePkg/Bus/Isa/Ps2MouseDxe/CommPs2.c
  MouseDev->State.RightButton = (UINT8) (RButton ? TRUE : FALSE);

  MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
  WholeFileSize = IS_FFS_FILE2 (FfsHeader) ? FFS_FILE2_SIZE (FfsHeader): FFS_FILE_SIZE (FfsHeader);
  
I guess we can sum it up in an oversimplified way as "where the
distance between the '=' and the '?' is short enough to make it
immediately clear that we're dealing with a ternary, and the test is
immediately obvious even to someone not familiar with the component in
question.

Nested ternaries are _always_ an abomination, even if trivial.

> And how do we ensure consistency across all EDK2 maintainers?

This is on my list for things to bring into the discussion in
Budapest.

Regards,

Leif