Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode()

["Laszlo Ersek" <lersek@redhat.com>]

On 07/16/19 03:18, Gao, Zhichao wrote:
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, July 16, 2019 5:56 AM
>> To: Gao, Liming <liming.gao@intel.com>; Gao, Zhichao
>> <zhichao.gao@intel.com>
>> Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
>> Marvin Häuser <mhaeuser@outlook.de>; Philippe Mathieu-Daudé
>> <philmd@redhat.com>
>> Subject: Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite
>> Base64Decode()
>>
>> On 07/15/19 17:22, Gao, Liming wrote:
>>> Laszlo:
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>>>> Laszlo Ersek
>>>> Sent: Saturday, July 13, 2019 3:31 AM
>>>> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Gao,
>>>> Liming <liming.gao@intel.com>; Kinney, Michael D
>>>> <michael.d.kinney@intel.com>
>>>> Cc: Marvin Häuser <mhaeuser@outlook.de>; Philippe Mathieu-Daudé
>>>> <philmd@redhat.com>
>>>> Subject: Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite
>>>> Base64Decode()
>>>>
>>>> On 07/12/19 04:31, Gao, Zhichao wrote:
>>>>> Sorry for late respond.
>>>>> The whole code is OK for me. And I write a tiny test for it without
>>>>> the memory address check. See
>>>>
>> https://github.com/ZhichaoGao/edk2/commit/615356ba32d3147957d215deb
>> d8
>>>> 44e7709f06849 . It is tested in Emulator environment. If it is OK, I think you
>> can take my Tested-by for this patch. If there are some missing, please let
>> me know.
>>>>
>>>> Thanks for writing that test app. It seems to have pretty good coverage.
>>>> I like that it covers the exit points systematically.
>>>>
>>>> Mike, Liming: I intend to pick up Zhichao's T-b, from above. If you
>>>> are aware of another test suite (perhaps used in conjunction with the
>>>> originally contributed Base64Decode() impl), please let me know.
>>>
>>> OK to me.
>>>
>>>>
>>>>> Base64Decode parameter Source is indicate as OPTIONAL. Although it
>>>>> is OK to be NULL, and return DestinationSize to be zero with
>>>> success status to indicate no decode occurred . I don't know if it is
>>>> meaningful to report the result as that. In my opinion, NULL Source is an
>> invalid input. Just my opinion, if the maintainer is OK with that, I am OK too.
>>>
>>> Yes. Source is NULL, SourceSize is zero. Code does nothing. I am fine with it.
>>>
>>> I have no other comments for the code logic. Reviewed-by: Liming Gao
>>> <liming.gao@intel.com>
>>
>> Thank you!
>>
>> A few questions:
>>
>>
>> (1) Liming, does your R-b apply to the first patch in the series as well? (That
>> patch too is for MdePkg.)
>>
>> Here are two links to patch#1 in the series, for your convenience:
>>
>> http://mid.mail-archive.com/20190702102836.27589-2-lersek@redhat.com
>> https://edk2.groups.io/g/devel/message/43162
>>
>>
>> (2) Zhichao, you made a good remark about block-scoped variables, and the
>> CCS. You also gave your Tested-by for the present version. So, we have the
>> following two options:
>>
>> (2a)
>>
>> - I push the present patch as-is, including your Tested-by. (Together with
>> Liming's R-b -- he is OK with the present version.)
>>
>> - Subsequently, I post a separate (incremental) patch for moving the
>> variables from the inner block scope to the outer function block scope.
>> This incremental patch needs another MdePkg maintainer review, but it
>> doesn't need additional testing from you.
>>
>> (2b) Alternatively:
>>
>> - I post v2 of this series, which incorporates the movement of the variables
>> from the inner block scope to the outermost function block scope.
>>
>> - I keep Liming's R-b. (The variable definition movement is straight-forward
>> and I don't think it invalidates Liming's R-b).
>>
>> - I drop your Tested-by, because the variable movement technically changes
>> code that your testing exercised, and a Tested-by tag should only be applied
>> to the *exact* code that was exercised by the testing.
>>
>> - Therefore, for preserving your testing work in the git history, you would
>> have to redo the testing, please.
>>
>>
>> Zhichao, do you prefer (2a) or (2b)?
>>
>> Personally, I prefer (2a), because (2a) is safe -- importantly, the v1 code is
>> *valid* C --, and it is less work for the community (for you, Liming and myself,
>> together).
> 
> (2a) makes less work and makes a good history, (2b) makes a clean commit message. I have no idea on which is better.
> For me, I prefer (2a) too because I don't think clean commit message is such important.

Wait, I think we have a misunderstanding here. *Both* (2a) and (2b) are
"fully clean" with regard to commit messages.

Under (2a), we will have three patches for MdePkg, in the end:

- specification update & present code removal (patch v1 1/3)
- reimplementation (patch v1 2/3), carrying Liming's R-b and your T-b
- separate update to clean up the location of the variable definitions.
  This needs Liming's R-b, but not your T-b.

Note that the "separate update" in the end does *not* invalidate your
T-b on the reimplementation patch. You *did* test that version, and your
T-b is attached to *that* version. Your T-b would not be attached to the
final (separate) small patch at all -- and that is alright. So it's all
faithful to reality.

Clean commit messages are extremely important to me. The point is that
(2a) and (2b) *both* satisfy that. In other words, it's not a
distinguishing factor between (2a) and (2b).

> It's up to you to decide. If you choose (2b) I am fine to do a test again and provide a T-B.

OK, I think I will go with (2a) then. But, I still need Liming's R-b (or
Mike's) on the first patch in the present series.

And Jordan's on the last one (but maybe I'll just defer the last patch,
for OvmfPkg, to a separate TianoCore BZ).

Thanks!
Laszlo