From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web10.85394.1597830459640207618 for ; Wed, 19 Aug 2020 02:47:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=NkhdC22K; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1597830458; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=zwIS1dxX2VE0BDvCuewFf92PsAOjxS4aPv7CLZ/3rH4=; b=NkhdC22KYH5v0Lp3ntSkKEhPzC6R8drnCiRBTNEHF3vG9WBcbMIaaLo668YCBIxeC/QglI Wb32EeGj3fElZhVpNpYYipeFIIluJEqUNZiCsdo8EEjbEyAGWfmeVX4DDjWcmM8/kd3Uy9 J6LBsPHdmgTzSqF2oRfsbI923SxeQgU= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-235-PXIxY2_oOhWg-hSo7SdTzQ-1; Wed, 19 Aug 2020 05:47:23 -0400 X-MC-Unique: PXIxY2_oOhWg-hSo7SdTzQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4FD5D81F02E; Wed, 19 Aug 2020 09:47:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-57.ams2.redhat.com [10.36.114.57]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0B44C1002391; Wed, 19 Aug 2020 09:47:18 +0000 (UTC) Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand To: Vladimir Olovyannikov , "Rabeda, Maciej" , "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> From: "Laszlo Ersek" Message-ID: <257b6d03-ff9f-4e9a-14f7-c537601a9061@redhat.com> Date: Wed, 19 Aug 2020 11:47:17 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <01083c792c35bba8472e4da75ee15867@mail.gmail.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0.001 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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