> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Tuesday, July 16, 2019 5:56 AM > To: Gao, Liming <liming....@intel.com>; Gao, Zhichao > <zhichao....@intel.com> > Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com>; > Marvin Häuser <mhaeu...@outlook.de>; Philippe Mathieu-Daudé > <phi...@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....@intel.com>; devel@edk2.groups.io; Gao, > >> Liming <liming....@intel.com>; Kinney, Michael D > >> <michael.d.kin...@intel.com> > >> Cc: Marvin Häuser <mhaeu...@outlook.de>; Philippe Mathieu-Daudé > >> <phi...@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....@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: > > 20190702102836.27589-2-lersek@redhat.com">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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
