Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: PlH3to+mgPMB90mU68GfXX9DSoH//O5JsuW1V2muoWcGChxlj31tfc6HkQg3oXhIJWG5eeBEGT
 rpo/lBPLsTxd37Hhw1fxm7jdr2BxLgBMTkLJSr4GeTWIMnoIoSGDTOtaRbt5PxDU2p/Ox5LBRt
 biSJ635ahtsYcPkay411pl/0i5vcTLnB0tWeRSn6oVrlpZ/dVlw/Aq20DcZbpYvo9VWV9tZxb2
 ctBaJgcDJ0KSyC9PQg5l33TaNTbS4s2KAbiAXCGDFOlPAcbWScIL2eaYFBvrrZuvc0SxzpSVPM
 TYg=
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To:     Christoph Hellwig <hch@lst.de>, Keith Busch <kbusch@kernel.org>,
        Sagi Grimberg <sagi@grimberg.me>
CC:     Niklas Cassel <Niklas.Cassel@wdc.com>, Jens Axboe <axboe@fb.com>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 0/2] remove workarounds for gcc bug wrt unnamed fields
 in initializers
Thread-Topic: [PATCH v2 0/2] remove workarounds for gcc bug wrt unnamed fields
 in initializers
Thread-Index: AQHWRats4KkNpfsaUkiEB9dxdlImtA==
Date:   Wed, 24 Jun 2020 22:40:21 +0000
Message-ID: <BYAPR04MB49655BD99E428B66EBB831E486950@BYAPR04MB4965.namprd04.prod.outlook.com>
References: <20200618200235.1104587-1-niklas.cassel@wdc.com>
 <20200624164441.GA24816@lst.de>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 7cb668ee-b530-46ac-617a-08d8188f93ce
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jun 2020 22:40:21.1151
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: JpiZ9a+jmfs5U121em5eTRILP6ciEQde6pgPPtkvzfzzOC6mVZY9D38xROC+dgTVsx+2n2dbdVjY77UaWDBTp3OjTA99K0vPaduV9ZzRrcs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB4664
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Christoph, Sagi and Keith,=0A=
=0A=
On 6/24/20 9:44 AM, Christoph Hellwig wrote:=0A=
> This looks good to me, but I'd rather wait a few releases to=0A=
> avoid too mush backporting pain.=0A=
> =0A=
=0A=
Here is a summary, for longer explanation please have a look at the=0A=
end [1] :-=0A=
=0A=
Pros:=0A=
1. Code looks uniform and follows strict policy.=0A=
=0A=
Cons:=0A=
1. Adds a tab + more char [1] which can lead to line breaks and that can=0A=
    be avoided without following declare-init pattern, less bugs and=0A=
    no pressure to fit the initializer in ~72 char given that we do have=0A=
    some long names and who knows what is in the future.=0A=
2. Issue with older version can lead to adding additional braces which=0A=
    does not look good.=0A=
3. Writing a new code becomes inflexible and pressure to fit initializer=0A=
    will not allow users to use meaningful names in the nested structures=
=0A=
    and anon unions.=0A=
4. Future patches will be needed for backward compatibility.=0A=
=0A=
Also code is perfectly readable as it is so why change ?=0A=
=0A=
If everyone is okay with above cons I'm fine adding this.=0A=
=0A=
Regards,=0A=
Chaitanya=0A=
=0A=
[1] Explanation :-=0A=
=0A=
I'm not against unifying the code. This will enforce struct =0A=
initialization to be done at the time of declaration and is inflexible =0A=
given that we have different transports and meaningful structure names.=0A=
Also, no one knows how many new structures will be coming since protocol =
=0A=
still has a room for improvement.=0A=
=0A=
Consider following :-=0A=
=0A=
e.g. 1=0A=
=0A=
static void nvme_xxx_func()=0A=
{=0A=
	struct nvme_XXX_YYY_ZZZ abcde {=0A=
		.member1  =3D line of the initializer calculation =3D X,=0A=
		.member2  =3D AAAAAAAAAAAAA + BBBBBBBBB + CCCCCC + DDDD +=0A=
			    EEEEE,=0A=
		.member3  =3D AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAa,=0A=
	}=0A=
}=0A=
=0A=
e.g. 2=0A=
=0A=
 From code :-=0A=
+	struct rdma_conn_param  param =3D {=0A=
+		.rnr_retry_count =3D 7,=0A=
+		.flow_control =3D 1,=0A=
+		.initiator_depth =3D min_t(u8, p->initiator_depth,=0A=
+	here ->>>queue->dev->device->attrs.max_qp_init_rd_atom),=0A=
+		.private_data =3D &priv,=0A=
+		.private_data_len =3D sizeof(priv),=0A=
+	};=0A=
=0A=
In above case (e.g.1, e.g.2) we loose 8 character =3D 1 tab for every =0A=
declaration-initialization, now if we have a member to be initialized =0A=
with complex calculations then it comes down to the next line and again =0A=
we loose 8 char of tab + (number of characters =3D name) of the member =0A=
(member2 in nvme_xx_func()) and whole things looks ugly, in contrast if =0A=
we do it outside of the declaration we still get 8 more characters =0A=
before we reach line limit. With 80 char limit we should avoid line =0A=
breaks and tabs as and when possible, this policy goes against it.=0A=
=0A=
=0A=