On Tue, 2021-03-09 at 17:44 +0000, Daniel P. Berrangé wrote: > One of the conventions we have had since the early days of libvirt is > that every struct typedef, has a corresponding "Ptr" typedef too. > > For example > > typedef struct _virDomainDef virDomainDef; > typedef virDomainDef *virDomainDefPtr; > > Periodically someone has questioned what the purpose of these Ptr > typedefs is, and we've not had an compelling answer, other than > that's what we've always done. > [...] > > Does anyone have suggestions for how these "Ptr" typedefs are > benefiting libvirt ? Would anyone miss them ? I consider them pointless obfuscation and would love to see them go. Note that I'm not talking just about humans either: some tooling also struggles a bit with the additional layer of indirection.

One of the conventions we have had since the early days of libvirt is that every struct typedef, has a corresponding "Ptr" typedef too. For example typedef struct _virDomainDef virDomainDef; typedef virDomainDef *virDomainDefPtr; Periodically someone has questioned what the purpose of these Ptr typedefs is, and we've not had an compelling answer, other than that's what we've always done. There are a few things that make me question the status quo - If we want a const pointer, we can't use const virDomainDefPtr def because that expands to "struct _virDomainDef * const", which is not what we need semantically. Instead we must write const virDomainDef *def It is not at all obvious why these two are different, but none the less they are, which is confusing to contributors To me this a compelling reason to consider the "Ptr" typedefs a waste of time, if not actively harmful. Please don't suggest adding virDomainDefConstPtr too ! - Writing 'virDomainDefPtr' is actually two characters more typing than 'virDomainDef *'. IOW these "Ptr" typedefs aren't saving us time when writing code.

One of the conventions we have had since the early days of libvirt is that every struct typedef, has a corresponding "Ptr" typedef too. For example typedef struct _virDomainDef virDomainDef; typedef virDomainDef *virDomainDefPtr;

We can't do anything about the use "Ptr" in the include/ files because that is public ABI. We can potentially eliminate "Ptr" types everywhere else in the codebase, even the src/libvirt*.c files corresponding to the public includes. Does anyone have suggestions for how these "Ptr" typedefs are benefiting libvirt ? Would anyone miss them ?

On 3/9/21 6:44 PM, Daniel P. Berrangé wrote: > One of the conventions we have had since the early days of libvirt is > that every struct typedef, has a corresponding "Ptr" typedef too. > > For example > >      typedef struct _virDomainDef virDomainDef; >      typedef virDomainDef *virDomainDefPtr; > > Periodically someone has questioned what the purpose of these Ptr > typedefs is, and we've not had an compelling answer, other than > that's what we've always done. > One possible upside is that it allows for less scary function headers, for instance: virDomainChrGetDomainPtrsInternal(virDomainDefPtr vmdef,                                   virDomainChrDeviceType type,                                   virDomainChrDefPtr ***arrPtr,                                   size_t **cntPtr); Three levels of dereference don't look as bad as four. But yeah, I agree we should drop them. I wonder if cocci can help here. Or even plain in-place sed, except we'd have to be careful with statements like:   virSomethingPtr a, b; where both @a and @b are pointers, and plain substitution would break it:   virSomething *a, b; (here only @a is poitner, ofc). But since we have Peter nagging about this kind of variable declaration, it's not something one couldn't fix by hand. And in fact, whilst we're at it we could declare each variable at its own line.

On 3/10/21 4:36 PM, Michal Privoznik wrote: > On 3/9/21 6:44 PM, Daniel P. Berrangé wrote: > > One of the conventions we have had since the early days of libvirt is > > that every struct typedef, has a corresponding "Ptr" typedef too. > > > > For example > > > >      typedef struct _virDomainDef virDomainDef; > >      typedef virDomainDef *virDomainDefPtr; > > > > Periodically someone has questioned what the purpose of these Ptr > > typedefs is, and we've not had an compelling answer, other than > > that's what we've always done. > > > > One possible upside is that it allows for less scary function headers, > for instance: > > virDomainChrGetDomainPtrsInternal(virDomainDefPtr vmdef, >                                   virDomainChrDeviceType type, >                                   virDomainChrDefPtr ***arrPtr, >                                   size_t **cntPtr); > > Three levels of dereference don't look as bad as four. > But yeah, I agree we should drop them. I wonder if cocci can help here. > Or even plain in-place sed, except we'd have to be careful with > statements like: > >   virSomethingPtr a, b; > > where both @a and @b are pointers, and plain substitution would break it: > >   virSomething *a, b; > > (here only @a is poitner, ofc). > > But since we have Peter nagging about this kind of variable declaration, > it's not something one couldn't fix by hand. And in fact, whilst we're > at it we could declare each variable at its own line. Okay, another problem: how does this g_auto() and similar work? I mean, now we have: G_DEFINE_AUTO_CLEANUP_FREE_FUNC(virSysinfoDefPtr, virSysinfoDefFree, NULL); and it works fine. But obviously either of: G_DEFINE_AUTO_CLEANUP_FREE_FUNC(virSysinfoDef *, ...); G_DEFINE_AUTO_CLEANUP_FREE_FUNC(virSysinfoDef, ...); is plain wrong. Maybe we can switch to g_autoptr() instead.