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

["Maciej Rabeda" <maciej.rabeda@linux.intel.com>]

@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.

Thanks,
Maciej

PS. Rabeda is my last name :)

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
>