From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mx.groups.io with SMTP id smtpd.web11.93618.1597859043174004409 for ; Wed, 19 Aug 2020 10:44:03 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 134.134.136.31, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: A7IOrDMrUzuOhAxF2ucHLI3a849h1/79m+aWX9LiTfYebbPAZukQQKCcHesfPbXjj/e03kegND G0J+mT3q5dKQ== X-IronPort-AV: E=McAfee;i="6000,8403,9718"; a="216695100" X-IronPort-AV: E=Sophos;i="5.76,332,1592895600"; d="scan'208";a="216695100" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Aug 2020 10:44:02 -0700 IronPort-SDR: hIpVcTv96SfP7xYyg1FGIxwtj68HB+0ujy/pTGj7cA1/Wpxk3417ofAt3LSyTf7Xevh1t4edjY oPA18vvGGZcA== X-IronPort-AV: E=Sophos;i="5.76,332,1592895600"; d="scan'208";a="472320455" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.249.158.203]) ([10.249.158.203]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Aug 2020 10:43:59 -0700 Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand 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 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> From: "Maciej Rabeda" Message-ID: <7686c2de-085c-68a6-7784-d2a93560fcc4@linux.intel.com> Date: Wed, 19 Aug 2020 19:43:52 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <257b6d03-ff9f-4e9a-14f7-c537601a9061@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: pl @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 >>> 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 >