Re: [PATCH] domain_capabilities: Assert enums fit into unsigned int bitmask
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.
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.
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.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>