[libvirt] [PATCH] conf: check rombar against VIR_DOMAIN_TRISTATE_SWITCH_ABSENT

[Michal Privoznik]

On 08/07/2017 05:30 PM, Pavel Hrdina wrote:
> On Mon, Aug 07, 2017 at 05:06:49PM +0200, Michal Privoznik wrote:
>> On 08/07/2017 04:56 PM, Ján Tomko wrote:
>>> Make the comparison explicit.
>>> ---
>>>  src/conf/domain_conf.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 3cdb5e348..b5ce2ecd9 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -5359,10 +5359,10 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>>>      }
>>>  
>>>      if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM) &&
>>> -        (info->rombar || info->romfile)) {
>>> +        (info->rombar != VIR_TRISTATE_SWITCH_ABSENT || info->romfile)) {
>>>  
>>>          virBufferAddLit(buf, "<rom");
>>> -        if (info->rombar) {
>>> +        if (info->rombar != VIR_TRISTATE_SWITCH_ABSENT) {
>>>              const char *rombar = virTristateSwitchTypeToString(info->rombar);
>>>  
>>>              if (rombar)
>>>
>>
>> I'm not against this patch, it's just that we set ABSENT explicitly to
>> zero value so that we can do shortcuts like this. If we don't want to
>> have them, we ought to remove the explicit value assignment.
> 
> The shortcut is nice, but I don't like it personally.  If the variable
> can contain more than two states I'd rather check it explicitly. 

So what's the point of assigning _ABSENT zero value then?

> That's
> why I prefer (int == 0) over (!int).

Well, if this is a part of bigger statement then yes, for instance:

if (x == 0) {
} else if (x == 1) {
} else {
}

(although, sometimes we might prefer switch() for that). But if it's
just a simple check whether a value was set or is equal to some default
(= if the check is interested in distinguishing just two states anyway),
!var works for me too:

if (x)
   formatToXML(x)

But sure, var == 0 vs !var is a personal preference. The important part
is my first question. If we dislike these shortcuts (in either of their
form), shouldn't we just drop explicit value assignment in the enum?

Michal