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 <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-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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#64393): https://edk2.groups.io/g/devel/message/64393
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]
-=-=-=-=-=-=-=-=-=-=-=-
