From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4E5B4210E38CC for ; Thu, 9 Aug 2018 04:29:37 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 92C5F40216EC; Thu, 9 Aug 2018 11:29:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-52.rdu2.redhat.com [10.10.121.52]) by smtp.corp.redhat.com (Postfix) with ESMTP id EA0292166BA2; Thu, 9 Aug 2018 11:29:35 +0000 (UTC) To: "Ni, Ruiyu" , "Dong, Eric" , "edk2-devel@lists.01.org" References: <20180808074006.21360-1-eric.dong@intel.com> <12da7e20-6f8e-0e3b-f026-42041e6b2415@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5BDC96E8@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <4fb40db5-0315-80b8-d9c6-7d1e11e65d9b@redhat.com> Date: Thu, 9 Aug 2018 13:29:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5BDC96E8@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 09 Aug 2018 11:29:36 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 09 Aug 2018 11:29:36 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Aug 2018 11:29:37 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/09/18 05:18, Ni, Ruiyu wrote: > We've reviewed the whole producer/consumer code and came to the conclusion > that 4GB/NVS restriction is unnecessary. I'd like to comment on this from a workflow POV as well. If you perform a detailed review of the code, that's great. However, in that case, please do take the time to *document* the review *in detail* in the commit message. I assume you and Eric may have spent a few hours reviewing the code, maybe drawing diagrams, using check lists, and so on. Why was none of that documented in the commit message? The commit message stated the result of your investigation, and none of the evidence. In order to review the patch, I had to rebuild the entire argument from zero, checking the life-cycle of every single field in ACPI_CPU_DATA, and that took the better part of a day. My job as a reviewer is to *read* your investigation and verify it, not to *reconstruct* it from scratch. Documenting one's findings in detail also helps one root out omissions or mistakes in one's reasoning. I catch my own errors like this all the time. Thanks Laszlo