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

On Mon, Aug 07, 2017 at 05:50:51PM +0200, Michal Privoznik wrote: > 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) IMHO we should use the same form for all checks. > > 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? The point is that the value 0 is still named as _ABSENT. The benefit of not using the shortcut for the VIR_TRISTATE_*_ABSENT is that you know right away what the variable stores and also you now right away that this part of code is executed only if the tristate was set to something.