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

[Gao, Zhichao]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek at redhat.com]
> Sent: Tuesday, July 16, 2019 5:56 AM
> To: Gao, Liming <liming.gao at intel.com>; Gao, Zhichao
> <zhichao.gao at intel.com>
> Cc: devel at edk2.groups.io; Kinney, Michael D <michael.d.kinney at intel.com>;
> Marvin Häuser <mhaeuser at outlook.de>; Philippe Mathieu-Daudé
> <philmd at 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 at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Saturday, July 13, 2019 3:31 AM
> >> To: Gao, Zhichao <zhichao.gao at intel.com>; devel at edk2.groups.io; Gao,
> >> Liming <liming.gao at intel.com>; Kinney, Michael D
> >> <michael.d.kinney at intel.com>
> >> Cc: Marvin Häuser <mhaeuser at outlook.de>; Philippe Mathieu-Daudé
> >> <philmd at 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 at 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.

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

Thanks,
Zhichao

> 
> Thanks!
> Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43755): https://edk2.groups.io/g/devel/message/43755
Mute This Topic: https://groups.io/mt/32284614/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-