Re: [Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features

[David Hildenbrand]

> On Mon, 8 Aug 2016 09:45:04 -0700 (PDT)
> address@hidden 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