From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from blyat.fensystems.co.uk (blyat.fensystems.co.uk [54.246.183.96]) by mx.groups.io with SMTP id smtpd.web12.6631.1621509092244717473 for ; Thu, 20 May 2021 04:11:33 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: ipxe.org, ip: 54.246.183.96, mailfrom: mcb30@ipxe.org) Received: from pudding.home (unknown [213.205.240.67]) by blyat.fensystems.co.uk (Postfix) with ESMTPSA id 16A4344214; Thu, 20 May 2021 11:11:28 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Remove hardcode 48 address size limitation To: devel@edk2.groups.io, lersek@redhat.com, "Ni, Ray" References: <12259.1621037062277250978@groups.io> <877e17dd-8235-1a56-13c1-c61a505d543e@redhat.com> From: "Michael Brown" Message-ID: <2891abbe-9df7-a2bc-a306-449910167513@ipxe.org> Date: Thu, 20 May 2021 12:11:22 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on blyat.fensystems.co.uk Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 18/05/2021 19:42, Laszlo Ersek wrote: > On 05/18/21 09:51, Ni, Ray wrote: >> Thanks for explaining why you don't think it's a good patch. I thought anytime changing a code, >> I should try to make it better, functional better, looks better. > > My only point was that separate concerns should be implemented in > separate patches, or at least (if they are really difficult, or > overkill, to isolate) that they should be documented. > > Please try to think with your reviewers' mindsets in mind, when > preparing a patch (commit message and code both). The question the patch > author has to ask themselves is not only "how do I implement this", but > also "how do I explain this to my reviewers". (Apologies in advance for intruding.) Ray: I think you may be neglecting one half of the problem. When making a code change, there are (at least) two requirements: 1. The new version of the code must make sense. You are definitely achieving this: as you say, "make it better, functional better, looks better". This is good. 2. The *change* between the old and new versions of the code (i.e. the patch and accompanying commit message) must also make sense. This is the requirement that I think Laszlo would like you to meet. It's not viable for anyone to meaningfully review code unless both of these requirements are met. Thanks, Michael