Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand

["Vladimir Olovyannikov" <vladimir.olovyannikov@broadcom.com>]

> -----Original Message-----
> From: Rabeda, Maciej <maciej.rabeda@linux.intel.com>
> Sent: Wednesday, August 19, 2020 10:44 AM
> To: Laszlo Ersek <lersek@redhat.com>; Vladimir Olovyannikov
> <vladimir.olovyannikov@broadcom.com>; Gao, Zhichao
> <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Wu, Jiaxin
> <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Gao, Liming <liming.gao@intel.com>; Nd
> <nd@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.rabeda@linux.intel.com>
> >>> Sent: Tuesday, August 18, 2020 9:54 AM
> >>> To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>;
> >>> Laszlo Ersek <lersek@redhat.com>; Gao, Zhichao
> >>> <zhichao.gao@intel.com>; devel@edk2.groups.io
> >>> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Wu,
> Jiaxin
> >>> <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Ni, Ray
> >>> <ray.ni@intel.com>; Gao, Liming <liming.gao@intel.com>; Nd
> >>> <nd@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
> >