Hi Rabeda, Thank you for reviewing. > -----Original Message----- > From: Rabeda, Maciej <maciej.rab...@linux.intel.com> > Sent: Tuesday, August 18, 2020 9:54 AM > To: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com>; Laszlo > Ersek <ler...@redhat.com>; Gao, Zhichao <zhichao....@intel.com>; > devel@edk2.groups.io > Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>; Wu, Jiaxin > <jiaxin...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Ni, Ray > <ray...@intel.com>; Gao, Liming <liming....@intel.com>; Nd > <n...@arm.com> > Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add > HttpDynamicCommand > > Hi Vladimir, > > I am inprog of going through the patch. There are some coding standard > violations. > For reference: > https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/ > > Http.c: > Lines 110-111: Spacing before open bracket and != operator Line 133: > Unnecessary brackets around EFI_SUCCESS Line 144+: Function argument > alignment, closed bracket spacing Line 357: End of function call ');' in a > multiline call should be in the next line, aligned with argument list Line > 360: In > a multiline if/while statement, '{' should be moved to a new line Line > 361, > 380, 388, ... : ShellPrintHiiEx calls: I suggest defining a macro to > improve > readability. First 3 parameters and mHttpHiiHandle seem to be commonly > used in your calls. > Line 740: Keep unary operation in a single line Lines 891-901, 946-1028: > Tab > alignment Lines 988-995: Assignments to a struct line by line. Could you > align > them in the following manner? > SpecificStruct.Field1 = 10; > SpecificStruct.Field2 = 20; > OK, I will look through all files again, probably something slipped from my attention as I work with Linux code as well.
> HttpApp.c: > Line 48: Space after type cast. Sorry, I don't understand here. You mean, Something = (CAST)SomethingElse should actually be "Something = (CAST) SomethingElse? I don't see this say in the Tftp.c: if (AsciiStrnCmp ((CHAR8 *)Option->OptionStr, "tsize", 5) == 0) ... or in TftpApp.c Status = (EFI_STATUS)RunTftp (ImageHandle, SystemTable); Am I missing anything? > > List above does not exhaust the things that can be found there. > May I ask you to read through the code again and catch what you and I > might > have missed? > > Additionally: > Http.c, TrimSpaces: > 1. It takes CHAR16** as an argument, yet it does not seem to modify the > value of the pointer for other functions outside its scope. I think it > would be > better to just make it CHAR16*. > 2. First while loop is highly inefficient. If I had a string like " > asdf.txt", it would > call CopyMem for each space, each time moving the asdf.txt one char closer > to the beginning. > It would be way better to achieve the same functionality by iterating from > front and back of the string looking for a char that is not a space nor a > tab. > Having their indices, you can easily call CopyMem once to move the string > forward and ZeroMem the rest. > Definitely, thanks. This TrimSpaces() was copy/paste from some other edk2 code. I should've looked into it. > Thanks, > Maciej Thank you, Vladimir > > On 18-Aug-20 00:52, Vladimir Olovyannikov wrote: > >> -----Original Message----- > >> From: Laszlo Ersek <ler...@redhat.com> > >> Sent: Monday, August 17, 2020 1:44 PM > >> To: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com>; > >> Rabeda, Maciej <maciej.rab...@linux.intel.com>; Gao, Zhichao > >> <zhichao....@intel.com>; devel@edk2.groups.io > >> Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>; Wu, > Jiaxin > >> <jiaxin...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Ni, Ray > >> <ray...@intel.com>; Gao, Liming <liming....@intel.com>; Nd > >> <n...@arm.com> > >> Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add > >> HttpDynamicCommand > >> > >> On 08/17/20 20:29, Vladimir Olovyannikov wrote: > >>>> -----Original Message----- > >>>> From: Laszlo Ersek <ler...@redhat.com> > >>>> Sent: Monday, August 17, 2020 11:01 AM > >>>> To: Rabeda, Maciej <maciej.rab...@linux.intel.com>; Vladimir > >>>> Olovyannikov <vladimir.olovyanni...@broadcom.com>; Gao, Zhichao > >>>> <zhichao....@intel.com>; devel@edk2.groups.io > >>>> Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>; Wu, > >> Jiaxin > >>>> <jiaxin...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Ni, Ray > >>>> <ray...@intel.com>; Gao, Liming <liming....@intel.com>; Nd > >>>> <n...@arm.com> > >>>> Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add > >>>> HttpDynamicCommand > >>>> > >>>> On 08/17/20 19:15, Rabeda, Maciej wrote: > >>>>> Hi Vladimir, > >>>>> > >>>>> I cannot apply the patch via 'git am'. > >>>>> Is your git configured in a manner described here? > >>>>> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unk > >>>>> em pt -git-guide-for-edk2-contributors-and-maintainers > >>> Hi Rabeda, > >>> Yes, I followed the page. I did not run the SetupGit.py though. > >>>>> > >>>>> Laszlo, > >>>>> > >>>>> Were you able to apply this patch from .eml file? > >>>> Yes, but I had to use some tricks (implemented by my colleague > >>>> Paolo in a python script) to undo the damage caused by the "quoted- > printable" > >>>> content-transfer-encoding on the posting. > >>>> > >>>> Our recommended Content-Transfer-Encoding (that is, > >>>> "sendemail.transferEncoding") is "8bit", or even "base64". > >>>> quoted-printable is practically impossible to get functional, with > >>>> the hard CRLF line endings in edk2. > >>>> > >>>> "BaseTools/Scripts/SetupGit.py" does set "8bit". > >>> Thank you for notice Laszlo, > >>> So, should I run this script and re-send the patch as v6? > >> I think that would be useful, yes -- and if you have made no changes > >> since v5, you can also post it as "v5 RESEND". If you've implemented > >> some changes meanwhile, then please post it as v6 indeed. > > Thanks Laszlo, > > I will post v6. > > > > Vladimir > >> Thanks > >> Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64398): https://edk2.groups.io/g/devel/message/64398 Mute This Topic: https://groups.io/mt/75826968/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
