From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f50.google.com (mail-lf1-f50.google.com [209.85.167.50]) by mx.groups.io with SMTP id smtpd.web12.93616.1597860411265486243 for ; Wed, 19 Aug 2020 11:06:52 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@broadcom.com header.s=google header.b=NPhjEYmG; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: broadcom.com, ip: 209.85.167.50, mailfrom: vladimir.olovyannikov@broadcom.com) Received: by mail-lf1-f50.google.com with SMTP id b30so12531728lfj.12 for ; Wed, 19 Aug 2020 11:06:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=from:references:in-reply-to:mime-version:thread-index:date :message-id:subject:to:cc; bh=yUe6vIA2JOKpHYn4/1WJw9MKyRx3rad+1s5F6+DR1Bs=; b=NPhjEYmGql47UpdDaOt9LmuNiaFGHHy45LMZWbqLc753tSsjHu+h2HtI0ZsHVWsZBX xwkYCPjZ26Xw7Dh15hHCtd00mrMja+bK2hGT9eN6tGHVVaxlfNPVDuwZaHLK9gUXTEp9 HPJ2FazuzXwL3+61sPgcdTTIcBA6/mCh6AI/c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:references:in-reply-to:mime-version :thread-index:date:message-id:subject:to:cc; bh=yUe6vIA2JOKpHYn4/1WJw9MKyRx3rad+1s5F6+DR1Bs=; b=NmGfM94vehD8p6wOKyrTHVOgqQymCR1PP1Aec3o6hEBDh9zn4dUYPvRrqUSfXxcb8c cFCtkhTTE/EvNF8BBQzbKRI83Q3zyxaHgy/0XodJ7ucs4UH788JYar7z3ZM9s7PM76C8 2O57GMqhBLIREXmMKQMuCHDSO1tO5JUF/Dh9HUZmgrvbv8+T4W/CYWGiXPG1HysYONoS 66TFiJl+bUv4lNCv0NokepJ89OwCBDN2HconQFdvnsCgas9IfpREl8V+A1T4Pb1JpkX8 kYOU/+O1eUYz0QxF8plxpnHr1ykeoVrIUvC8UzKt5HuATwXRUJ2cr72dsFXVwQcNblaG NO2w== X-Gm-Message-State: AOAM530zgXcxkjvqVqKLHzRtUh64dawx0fvhbkaujoeHckDgnHbWL5Ru bh4K7A+32u8ZowwAGsS3fH+5alTCVd7fccbh1uh/zQ== X-Google-Smtp-Source: ABdhPJzqi/Q9ialHuAfGP7OkDv4WDRuQfJHg1AeM9SecI5weLDUVj7io9/B9At4EeM96OGtpLCdX0j5CJwr4urR+eCo= X-Received: by 2002:a19:a07:: with SMTP id 7mr12760396lfk.65.1597860409278; Wed, 19 Aug 2020 11:06:49 -0700 (PDT) From: "Vladimir Olovyannikov" References: <20200727164830.25829-1-vladimir.olovyannikov@broadcom.com> <20200727164830.25829-2-vladimir.olovyannikov@broadcom.com> <25b5ce9498095e68650b8817e4523714@mail.gmail.com> <2a710789-bfdd-dd80-386a-04fbd46d4835@redhat.com> <01083c792c35bba8472e4da75ee15867@mail.gmail.com> <257b6d03-ff9f-4e9a-14f7-c537601a9061@redhat.com> <7686c2de-085c-68a6-7784-d2a93560fcc4@linux.intel.com> In-Reply-To: <7686c2de-085c-68a6-7784-d2a93560fcc4@linux.intel.com> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQILIYHslp9C8beoIeXBb+C1khe8nAJZRV/xAGCCW/QCSmia/gFipDoLAhIip3QCjNqSBgK2ARQXAj2BhEYBjU0/KgICYUwjAcLYP5UChMtntqgX3LWg Date: Wed, 19 Aug 2020 11:06:46 -0700 Message-ID: Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand To: "Rabeda, Maciej" , Laszlo Ersek , "Gao, Zhichao" , devel@edk2.groups.io Cc: Samer El-Haj-Mahmoud , "Wu, Jiaxin" , "Fu, Siyuan" , "Ni, Ray" , "Gao, Liming" , Nd Content-Type: text/plain; charset="UTF-8" > -----Original Message----- > From: Rabeda, Maciej > Sent: Wednesday, August 19, 2020 10:44 AM > To: Laszlo Ersek ; Vladimir Olovyannikov > ; Gao, Zhichao > ; devel@edk2.groups.io > Cc: Samer El-Haj-Mahmoud ; Wu, Jiaxin > ; Fu, Siyuan ; Ni, Ray > ; Gao, Liming ; Nd > > 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 > >>> Sent: Tuesday, August 18, 2020 9:54 AM > >>> To: Vladimir Olovyannikov ; > >>> Laszlo Ersek ; Gao, Zhichao > >>> ; devel@edk2.groups.io > >>> Cc: Samer El-Haj-Mahmoud ; Wu, > Jiaxin > >>> ; Fu, Siyuan ; Ni, Ray > >>> ; Gao, Liming ; Nd > >>> > >>> 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 > >