From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web10.25109.1574349323082756793 for ; Thu, 21 Nov 2019 07:15:23 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=J8yIVHKL; spf=pass (domain: linaro.org, ip: 209.85.221.68, mailfrom: leif.lindholm@linaro.org) Received: by mail-wr1-f68.google.com with SMTP id s5so4895258wrw.2 for ; Thu, 21 Nov 2019 07:15:22 -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:in-reply-to:user-agent; bh=sHn7o12CQMU+iI+9FhhrOM6iidlLZYrFkm8DINYtluc=; b=J8yIVHKL9YBbxysnuw6fO7f5bNvDC9aBtRtQAbsjIQZq0A78i2mHO6PUEh86s0mz+p YuCzHAUV38dRNqdfaTNg2f0FGJUFoj5mo/CrAZNEDsGhwCwnAbw9Wno1+IoD8ToAFdx3 hyN+TJ97o9yu06Mtegmlk53d1t081fIE0KgP/8RtqaK6oTjRI6ISoUj1TADp/dylzs/f VhmJ6V/TeVmc0izFmipcb2sKWIACNoUR1Xhm0DrijRmqQ4pcoOtAVq8JwZ4U/sFbNeKC cHzKbp47IDQBaYOqMTDkSIIXBJrRIVeZEajuOwuCMQcZXw21kbLE92WXZXBgzBkOe/Px 33pg== 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:in-reply-to:user-agent; bh=sHn7o12CQMU+iI+9FhhrOM6iidlLZYrFkm8DINYtluc=; b=byCwFaToVhHRplDfv+QT3p0Aa8ib3VpSAzs9EmBA4YTY2wcxWBNYLEhvS7qkAF6E89 d/Rzqpz8cnY5ac+Ld1iFvfRpiwTLHcbht+QhzCTmQVdziKiPOCiWAomtLAi13uO5sHpZ bVrAbTxGwMWO5Yz0yvayd9O8mKYdL8gcahWemgKvPu/ZhyUF8HQWUjp4Th7EvjcrX7kP uByOlP4ZAOs85ZoM7KnWlTSlCGNIeA4SUNQhLNiJiIo8MwUCymwlkO19zhTaMvBd2HHl vfeyRlzf0tNfpK6Z/Fj/1Mpgjx8s52R8a6UqOjJb+VxCl+06cyvxZYnVYO6CBDgqGOPz OJcw== X-Gm-Message-State: APjAAAU572qEOAMGpIz09Efon6FMS0symMOMDnTHg4j1IwKTrVvAA96R MwBBcHfh0eM6x02Rfv/KAT7c4Q== X-Google-Smtp-Source: APXvYqzQ6uO265OVXvmwXTBO0T+U/Mc0NUJ9s9PyvfLIgQDAu1cE23/+BJ7yTgJ40qDD9/SlPLueZg== X-Received: by 2002:adf:dc90:: with SMTP id r16mr11750583wrj.258.1574349321422; Thu, 21 Nov 2019 07:15:21 -0800 (PST) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 16sm9529601wmf.0.2019.11.21.07.15.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 Nov 2019 07:15:20 -0800 (PST) Date: Thu, 21 Nov 2019 15:15:19 +0000 From: "Leif Lindholm" To: Ard Biesheuvel Cc: Sami Mujawar , edk2-devel-groups-io , Alexei Fedorov , Matteo Carlini , nd Subject: Re: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning Message-ID: <20191121151519.GG7359@bivouac.eciton.net> References: <20190823105539.13260-1-sami.mujawar@arm.com> <20190823105539.13260-19-sami.mujawar@arm.com> <20191121123542.GD7359@bivouac.eciton.net> <20191121125315.GF7359@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Nov 21, 2019 at 14:09:41 +0100, Ard Biesheuvel wrote: > > > I don't like this at all. > > > > > > If two expressions happen to evaluate to the same constant, we should > > > be able to compare them using C code. > > > > > > I think we should disable the warning instead. > > > > I agree with that as a general rule, and agree that disabling the > > warning may be preferable in other contexts. > > > > But I somewhat disagree with referring to *this instance* as two > > expressions evaluating to the same constant. This is entirely a > > preprocessor event, somewhat obscured by the Pcd syntax. > > The test is "has the default baudrate been requested or has a specific > > one been requested?". > > That may be true, but moving from C conditionals to CPP conditionals > obscures the code, and reduces test coverage, so I think it should be > avoided when possible. I don't see what test coverage this would change. Even in the NOOPT case, only the codegen would be affected - only one of the two cases would be reachable. In all other cases, the compiler would go if (115200 != 0) { /* Oh, I'll just ignore any else clauses then ... */ I think the pre-existing code obscures what is actually going on and that the change clarifies it (but could be prettified). Now, of course, why we have a platform-specific (and platform-global) override variable to make a library ignore its inputs is another interesting question. So. Rock-paper-scissors or coin-toss? / Leif