> On Mon, 8 Aug 2016 09:45:04 -0700 (PDT) > no-re...@patchew.org wrote: > > <Trimmed the no-reply@ -- I wonder whether reply-to should be setup to > 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