From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x236.google.com (mail-wm0-x236.google.com [IPv6:2a00:1450:400c:c09::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 66A3482167 for ; Fri, 24 Feb 2017 06:06:20 -0800 (PST) Received: by mail-wm0-x236.google.com with SMTP id v77so15226610wmv.1 for ; Fri, 24 Feb 2017 06:06:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=76VdFG/2MKPWTLf00PGkbwWrfYqi8KyJGwS/DFGCrGE=; b=XJqVtepUK5HsL7+vZM+p79luq8xnP+106cJ0sAbvwTHRmQSJf+1VWylfRxeSbCTuc3 gBxYINELQ7aRyO6T9EVHm5oXkASaB+Z0JH8Eud4MUTYhD8GN6eBBFEbGQoNJPVugmYtx Z0uPEszPFVTuUC6dJxKHE1EvXHRCITOJFc6RA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=76VdFG/2MKPWTLf00PGkbwWrfYqi8KyJGwS/DFGCrGE=; b=nsrPzC9xPSG/cZ9yDYxt5NiK9a1ObWk0ZNInaaw/ecPZQLujaTqp5v7iNC4ri3+7Ku 6vnC+JCike5f4d+LrSGJzdsrSLcxPXfIz40GdHHXeBLxANAQZFplWlltgSvYFbjQxcpq 02BQ3BnyphiwoIFejCq0JUezfSw85qcpx0xgGwifJlCoCj6O3XKIaOy2XIEDwY+xhTtJ ENsqpYpW8wrJxNfqa0HOibB/Smn5TbyDurMALCOGxeN4vYAbmokrdq6gH89lcSUH7bUS kDWsinwPFvHxRkKQgEVm1JBS9oIRFNJN/UqguMZ+WboLjiJre88YUrCVqqG2xRabGuiX 51RA== X-Gm-Message-State: AMke39kcnh8yivcLyXEO+Ed6anohoLGkUmFQdiyiSx95aK8iM0ax2/6nlJrxWKp/uQ/yUoHZ X-Received: by 10.28.48.7 with SMTP id w7mr2932470wmw.78.1487945178760; Fri, 24 Feb 2017 06:06:18 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id r5sm10479053wra.25.2017.02.24.06.06.17 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 24 Feb 2017 06:06:17 -0800 (PST) Date: Fri, 24 Feb 2017 14:06:16 +0000 From: Leif Lindholm To: Evan Lloyd Cc: "ryan.harkin@linaro.org" , "ard.biesheuvel@linaro.org" , "edk2-devel@ml01.01.org" Message-ID: <20170224140616.GC16034@bivouac.eciton.net> References: <20170209192623.262044-1-evan.lloyd@arm.com> <20170209192623.262044-5-evan.lloyd@arm.com> <20170213121552.GU16034@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Feb 2017 14:06:20 -0000 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit (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