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

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

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;

HttpApp.c:
Line 48: Space after type cast.

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.

Thanks,
Maciej

On 18-Aug-20 00:52, Vladimir Olovyannikov wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Monday, August 17, 2020 1:44 PM
>> To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>;
>> Rabeda, Maciej <maciej.rabeda@linux.intel.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
>>
>> On 08/17/20 20:29, Vladimir Olovyannikov wrote:
>>>> -----Original Message-----
>>>> From: Laszlo Ersek <lersek@redhat.com>
>>>> Sent: Monday, August 17, 2020 11:01 AM
>>>> To: Rabeda, Maciej <maciej.rabeda@linux.intel.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
>>>>
>>>> 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-unkem
>>>>> 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