[libvirt] [PATCH] conf: check rombar against VIR_DOMAIN_TRISTATE_SWITCH_ABSENT
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) -- 2.13.0
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. Michal
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. That's why I prefer (int == 0) over (!int). Pavel
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
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. Yes, it this particular case you can see few lines below that there is virTristateSwitchTypeToString() which gives you hint what the variable is, but it's not always that way. Pavel
On 08/08/2017 10:49 AM, Pavel Hrdina wrote:
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.
This is argument for being explicit about checks (which I'm not objecting to, I said at the beginning that I'm up for it). The thing I'm interested in is: if we are explicit in all the places, what's the point in having explicit assignment _ABSENT = 0? We have some enums without explicit value assignment to any member. The only reason I can think of is that when we alloc new struct, the memory chunk is prefilled with zeroes so we don't have to do explicit setting to _ABSENT. Michal
participants (3)
-
Ján Tomko -
Michal Privoznik -
Pavel Hrdina