On 07/16/19 03:18, Gao, Zhichao wrote: > > >> -----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.
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43784): https://edk2.groups.io/g/devel/message/43784 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] -=-=-=-=-=-=-=-=-=-=-=-
