Re: [Qemu-devel] [qemu-s390x] [PATCH 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable

[Halil Pasic]

On 02/20/2018 05:25 PM, Cornelia Huck wrote:
> On Tue, 20 Feb 2018 17:08:52 +0100
> David Hildenbrand <address@hidden> wrote:
> 
>> On 20.02.2018 17:07, Cornelia Huck wrote:
>>> On Tue, 20 Feb 2018 17:04:19 +0100
>>> Christian Borntraeger <address@hidden> wrote:
>>>   
>>>> On 02/20/2018 04:55 PM, David Hildenbrand wrote:  
>>>>> On 20.02.2018 16:53, Cornelia Huck wrote:    
>>>>>> On Tue, 20 Feb 2018 16:07:13 +0100
>>>>>> Halil Pasic <address@hidden> wrote:
>>>>>>    
>>>>>>> The 'bit' field of the 'S390FeatDef' structure is not applicable to all
>>>>>>> it's instances. Currently a this field is not applicable, and remains   
>>>>>>>  
>>>>>>
>>>>>> s/it's/its/
>>>>>>
>>>>>> s/a this/this/
>>>>>>    
>>>>>>> unused, iff the feature is of type S390_FEAT_TYPE_MISC. Having the 
>>>>>>> value 0
>>>>>>> specified for multiple such feature definition  was a little confusing,
>>>>>>> as it's a perfectly legit bit value, and as usually the value of the bit
>>>>>>> field is ought to be unique for each feature.
>>>>>>>
>>>>>>> Let's document this, and hopefully reduce the potential for confusion.
>>>>>>>
>>>>>>> Signed-off-by: Halil Pasic <address@hidden>
>>>>>>> ---
>>>>>>>
>>>>>>> Hi!
>>>>>>>
>>>>>>> This may be an overkill. A comment where the misc features
>>>>>>> are defined would do to, but I think this is nicer. So
>>>>>>> I decided to try it with this approach first.    
>>>>>>
>>>>>> Is there likely to be anything else than FEAT_MISC _not_ using .bit? If
>>>>>> not, would it be better to at a comment to the FEAT_MISC definition?    
>>>>>
>>>>> Doubt it right now. I would sign the "overkill" part :)    
>>>>
>>>> I can cconfirm that this code caused some questions and it took me some
>>>> minutes to remember why 0 and 0 was ok. So I certainly want to have a 
>>>> comment
>>>> of some form.
>>>>  
>>>
>>> I'd prefer a comment about FEAT_MISC usage rather than a magic value.
>>>   
>>
>> We can also add FEAT_INIT_MISC. And add a comment in the initializer.
>>
> 
> That's what I like best.
> 

OK, seems we have a winner: I will redo this with
#define FEAT_INIT_MISC(_name, _desc)                 \                          
            FEAT_INIT(_name, S390_FEAT_TYPE_MISC, 0, _desc)

Everybody thanks for the comments.

Regards,
Halil