From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46053) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bWo7w-0006kN-Ia for qemu-devel@nongnu.org; Mon, 08 Aug 2016 13:14:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bWo7s-0000WX-CW for qemu-devel@nongnu.org; Mon, 08 Aug 2016 13:14:11 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:22267 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bWo7s-0000WO-6b for qemu-devel@nongnu.org; Mon, 08 Aug 2016 13:14:08 -0400 Received: from pps.filterd (m0098413.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u78H5ZwA011932 for ; Mon, 8 Aug 2016 13:14:07 -0400 Received: from e06smtp12.uk.ibm.com (e06smtp12.uk.ibm.com [195.75.94.108]) by mx0b-001b2d01.pphosted.com with ESMTP id 24na1wkvwd-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 08 Aug 2016 13:14:07 -0400 Received: from localhost by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 8 Aug 2016 18:14:06 +0100 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 27BC2219004D for ; Mon, 8 Aug 2016 18:13:30 +0100 (BST) Received: from d06av05.portsmouth.uk.ibm.com (d06av05.portsmouth.uk.ibm.com [9.149.37.229]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u78HE4Yt17432788 for ; Mon, 8 Aug 2016 17:14:04 GMT Received: from d06av05.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av05.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u78HE4H1027156 for ; Mon, 8 Aug 2016 11:14:04 -0600 Date: Mon, 8 Aug 2016 19:14:01 +0200 From: David Hildenbrand In-Reply-To: <20160808190203.2f8c8a60.cornelia.huck@de.ibm.com> References: <1470670378-53732-1-git-send-email-dahi@linux.vnet.ibm.com> <201608081645.u78GKFHE092220@mx0b-001b2d01.pphosted.com> <20160808190203.2f8c8a60.cornelia.huck@de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-Id: <20160808191401.63a8d4c4@thinkpad-w530> Subject: Re: [Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: qemu-devel@nongnu.org, famz@redhat.com, ehabkost@redhat.com, borntraeger@de.ibm.com, fiuczy@linux.vnet.ibm.com, imammedo@redhat.com, jdenemar@redhat.com, mimu@linux.vnet.ibm.com > On Mon, 8 Aug 2016 09:45:04 -0700 (PDT) > no-reply@patchew.org wrote: > > something sensible, e.g. qemu-devel> > > > Checking PATCH 4/29: s390x/cpumodel: introduce CPU features... > > WARNING: line over 80 characters > > #65: FILE: target-s390x/cpu_features.c:27: > > + FEAT_INIT("zarch", S390_FEAT_TYPE_STFL, 1, "z/Architecture architectural mode"), > > We could conceivably break the line after one of the arguments, but > there are some very long strings below. I'm not a fan of very long > lines myself, but I wonder whether we should relax that limit? The > Linux kernel has settled upon a relaxed limit and forbids splitting > strings for greppability. I kept these > 80 as these definitions look much cleaner this way. > > > > Checking PATCH 5/29: s390x/cpumodel: generate CPU feature lists for CPU models... > > ERROR: Macros with complex values should be enclosed in parenthesis > > #113: FILE: target-s390x/gen-features.c:20: > > +#define S390_FEAT_GROUP_PLO \ > > + S390_FEAT_PLO_CL, \ > > + S390_FEAT_PLO_CLG, \ > > + S390_FEAT_PLO_CLGR, \ > > + S390_FEAT_PLO_CLX, \ > > + S390_FEAT_PLO_CS, \ > > + S390_FEAT_PLO_CSG, \ > > + S390_FEAT_PLO_CSGR, \ > > + S390_FEAT_PLO_CSX, \ > > + S390_FEAT_PLO_DCS, \ > > + S390_FEAT_PLO_DCSG, \ > > + S390_FEAT_PLO_DCSGR, \ > > + S390_FEAT_PLO_DCSX, \ > > + S390_FEAT_PLO_CSST, \ > > + S390_FEAT_PLO_CSSTG, \ > > + S390_FEAT_PLO_CSSTGR, \ > > + S390_FEAT_PLO_CSSTX, \ > > + S390_FEAT_PLO_CSDST, \ > > + S390_FEAT_PLO_CSDSTG, \ > > + S390_FEAT_PLO_CSDSTGR, \ > > + S390_FEAT_PLO_CSDSTX, \ > > + S390_FEAT_PLO_CSTST, \ > > + S390_FEAT_PLO_CSTSTG, \ > > + S390_FEAT_PLO_CSTSTGR, \ > > + S390_FEAT_PLO_CSTSTX > > I think the check is wrong to complain here. Yes, these complaints are wrong. > > > Checking PATCH 6/29: s390x/cpumodel: generate CPU feature group lists... > > Checking PATCH 7/29: s390x/cpumodel: introduce CPU feature group definitions... > > WARNING: line over 80 characters > > #71: FILE: target-s390x/cpu_features.c:363: > > + FEAT_GROUP_INIT("gen13ptff", GEN13_PTFF, "PTFF enhancements introduced with z13"), > > Same comments as above. Dito, the list of features just looks cleaner. [...] > > > Checking PATCH 27/29: s390x/cpumodel: implement QMP interface "query-cpu-model-expansion"... > > WARNING: line over 80 characters > > #152: FILE: target-s390x/cpu_models.c:408: > > + s390_feat_bitmap_to_ascii(model->features, qdict, qdict_add_enabled_feat); I'll fix that > > > > WARNING: line over 80 characters > > #165: FILE: target-s390x/cpu_models.c:421: > > +CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType type, > > > > ERROR: space prohibited before that close parenthesis ')' > > #181: FILE: target-s390x/cpu_models.c:437: > > + } else if (type != CPU_MODEL_EXPANSION_TYPE_FULL ) { > > That's actually a warning I concur with :) And this of course :) > > I think we should at least come up with a check for a linux header > update to avoid spamming the mailing list. And it would be a good idea > to have some consensus about the 80 char limit as well. For the definitions and the lengthy function prototypes I really don't want to break readability just to conform to 80 chars. Thanks. David