Re: [PATCH] domain_capabilities: Assert enums fit into unsigned int bitmask
On 11/19/20 12:47 PM, Erik Skultety wrote:
On Thu, Nov 19, 2020 at 12:22:07PM +0100, Michal Privoznik wrote:
> On 11/19/20 9:48 AM, Erik Skultety wrote:
>
>>> where VIR_DECLARE_MEMBER() would assert() that 1 <<
>>> VIR_DOMAIN_VIDEO_TYPE_LAST fits into 'unsigned int' (or whatever type
we
>>> will use), and also declare 'modelType' member for the struct.
I'm not sure
>>> a tags generator would be capable of seeing that.
>>
>> Why do you have to do both at the same time? Why not putting the assert right
>> after the virDomainVideoType enum typedef? I think it's much more obvious if
>> the guard comes right after what it actually guards.
>
> You mean to put the assert into domain_conf.h? I'm not sure it would make
> sense. Because the assert actually guides virDomainCapsEnum struct and the
> way we store info there and not virDomainVideoType per se. For instance for
> enums not (yet) exposed on domCaps we don't care about their size.
>
I was also not opting to guard all of them.
I know, but my point was, if I opened domain_conf.h and saw asserts only
for some enums it would keep me wondering what is going on and why the
rest doesn't have asserts.
> Moreover, if I leave those asserts where they are now, then during
> compilation the compiler reports error right where the problem is and not at
> a place where it leaves you wondering why the assert is there.
I'd say the confusion rate is about the same as when you're guarding several
virDomainCapsEnum variables, none of which has the corresponding type mentioned
in a commentary right next to it.
The thing is, you can't insert G_STATIC_ASSERT into a struct definition,
it doesn't compile:
struct _virDomainCapsDeviceVideo {
virTristateBool supported;
virDomainCapsEnum modelType; /* virDomainVideoType */
STATIC_ASSERT_ENUM(VIR_DOMAIN_VIDEO_TYPE_LAST);
};
/usr/include/glib-2.0/glib/gmacros.h:745:31: error: expected
specifier-qualifier-list before ‘typedef’
745 | #define G_STATIC_ASSERT(expr) typedef char G_PASTE
(_GStaticAssertCompileTimeAssertion_, __COUNTER__)[(expr) ? 1 : -1]
G_GNUC_UNUSED
| ^~~~~~~
../src/conf/domain_capabilities.h:40:5: note: in expansion of macro
‘G_STATIC_ASSERT’
40 | G_STATIC_ASSERT(last <= sizeof(unsigned int) * CHAR_BIT)
| ^~~~~~~~~~~~~~~
../src/conf/domain_capabilities.h:96:5: note: in expansion of macro
‘STATIC_ASSERT_ENUM’
96 | STATIC_ASSERT_ENUM(VIR_DOMAIN_VIDEO_TYPE_LAST);
| ^~~~~~~~~~~~~~~~~~
That leaves us with only two options: before of after the struct.
On the other hand, I can see the value and convenience in copy-paste
- when
one needs to add a new domain capability, they see that all the other struct
definitions are guarded, so they do the same, hard to argue against that...
compared to when you need not to forget to guard the enum that gets propagated
into the capability.
Fair enough, but make sure you leave out the assert for VIR_TRISTATE_BOOL, I
mean we can guarantee that this one will never magically acquire another 29
values.
In this case I'd prefer consistency. Surely, verifying one assert() at
build time is taking no time.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
Michal