> -----Original Message----- > From: Rabeda, Maciej <maciej.rab...@linux.intel.com> > Sent: Wednesday, August 19, 2020 10:44 AM > To: Laszlo Ersek <ler...@redhat.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 > > @Laszlo, > > As for type casting, I don't have strong opinions. > Space after typecast (for me) always seemed to be visually more consistent > with the rest of the code. > In my regular projects written in C-based languages, I would do it the way > you are describing. > > @Vladimir > I went through the rest of the code. > There are more coding standard problems, but in terms of HTTP usage the > code seems to be OK. OK, great, thanks. I am reviewing it and will submit the patch shortly. Thanks to Laszlo, I hope to be able to run CI locally and detect hidden issues. Indeed, I have to switch between Linux and UEFI code, so I missed several code style issues. > > Thanks, > Maciej > > PS. Rabeda is my last name :) Oops, thanks for letting me know, Maciej. It is just a bit unusual to have the last name first :)
Thank you, Vladimir > > On 19-Aug-20 11:47, Laszlo Ersek wrote: > > On 08/18/20 20:33, Vladimir Olovyannikov wrote: > >>> -----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 > >>> / > > [...] > > > >> OK, I will look through all files again, probably something slipped > >> from my attention as I work with Linux code as well. > > Now that we have ECC checking enabled in CI, submitting a personal > > pull request could help. Additionally, the CI workload should be > > possible to run locally, according to ".pytool/Readme.md". > > > > (I'm planning to learn how to run that locally myself.) > > > >>> 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? > > > > In core edk2 packages, casts are frequently written like this: > > > > (TYPE) Expression > > > > (Importantly, Expression itself is not parenthesized.) > > > > In my opinion, this is a bad practice. That's because the typecast has > > one of the strongest operator bindings in the C language, and visually > > distancing (TYPE) from "Expression" suggests the opposite -- it's > > counter-intuitive. > > > > I strongly prefer > > > > (TYPE)Expression > > > > but other maintainers may have a different preference. > > > > Thanks > > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64459): https://edk2.groups.io/g/devel/message/64459 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] -=-=-=-=-=-=-=-=-=-=-=-
