From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <jordan.l.justen@intel.com>
Received: from mga06.intel.com (mga06.intel.com [134.134.136.31])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by ml01.01.org (Postfix) with ESMTPS id B5192802A0
 for <edk2-devel@lists.01.org>; Fri, 10 Mar 2017 02:14:47 -0800 (PST)
Received: from fmsmga003.fm.intel.com ([10.253.24.29])
 by orsmga104.jf.intel.com with ESMTP; 10 Mar 2017 02:14:47 -0800
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.36,140,1486454400"; d="scan'208";a="833098288"
Received: from mpvats-mobl2.amr.corp.intel.com (HELO localhost)
 ([10.254.184.25])
 by FMSMGA003.fm.intel.com with ESMTP; 10 Mar 2017 02:14:46 -0800
MIME-Version: 1.0
To: Laszlo Ersek <lersek@redhat.com>, edk2-devel-01 <edk2-devel@lists.01.org>
Message-ID: <148914088572.26490.12215430240628550434@jljusten-skl>
From: Jordan Justen <jordan.l.justen@intel.com>
In-Reply-To: <20170223014814.10937-12-lersek@redhat.com>
References: <20170223014814.10937-1-lersek@redhat.com>
 <20170223014814.10937-12-lersek@redhat.com>
User-Agent: alot/0.5.1
Date: Fri, 10 Mar 2017 02:14:45 -0800
Subject: Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
X-BeenThere: edk2-devel@lists.01.org
X-Mailman-Version: 2.1.21
Precedence: list
List-Id: EDK II Development  <edk2-devel.lists.01.org>
List-Unsubscribe: <https://lists.01.org/mailman/options/edk2-devel>,
 <mailto:edk2-devel-request@lists.01.org?subject=unsubscribe>
List-Archive: <http://lists.01.org/pipermail/edk2-devel/>
List-Post: <mailto:edk2-devel@lists.01.org>
List-Help: <mailto:edk2-devel-request@lists.01.org?subject=help>
List-Subscribe: <https://lists.01.org/mailman/listinfo/edk2-devel>,
 <mailto:edk2-devel-request@lists.01.org?subject=subscribe>
X-List-Received-Date: Fri, 10 Mar 2017 10:14:47 -0000
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On 2017-02-22 17:48:13, Laszlo Ersek wrote:
> +  //
> +  ScratchBuffer->Features =3D mSmiFeatures;
> +  Status =3D QemuFwCfgS3WriteBytes (mRequestedFeaturesItem,

I noticed a fair amount of function calls where the first args are
appearing on the first line, and then the rest appear on subsequent
lines indented two spaces.

> +             sizeof ScratchBuffer->Features);
> +  if (RETURN_ERROR (Status)) {
> +    goto FatalError;
> +  }
> +

I can't say that the coding style prohibits this, but it does seem odd
looking. I would expect to see:

[1]

  Status =3D QemuFwCfgS3WriteBytes (
             mRequestedFeaturesItem,
             sizeof ScratchBuffer->Features
             );

The coding style examples look like this:

  Status =3D QemuFwCfgS3WriteBytes (
             mRequestedFeaturesItem,
             sizeof ScratchBuffer->Features
           );

But this looks less like what I'm used to actually seeing in EDK II.
(Look at DXE Core, for example. It looks like [1].)

I personally think this is okay to save a line, but it doesn't seem to
follow the coding style doc:

  Status =3D QemuFwCfgS3WriteBytes (
             mRequestedFeaturesItem,
             sizeof ScratchBuffer->Features);

Now, we all know EDK II has a style of it's own, but outide EDK II, I
might expect something like:

  Status =3D QemuFwCfgS3WriteBytes(mRequestedFeaturesItem,
                                 sizeof ScratchBuffer->Features);

Which once again shows arguements on subsequent lines are lined up.
Based on that, I think [1] is the best style for EDK II code.

-Jordan