RFC: do we want/need the "Ptr" typedefs for internal 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; 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. - This convention of having "Ptr" typedefs is atypical among C projects I've worked on. Anything that is peculiar to libvirt is another item that new contributors need to learn, so has a cost to the project that must be weighed against its benefit. 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 ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Mar 09, 2021 at 07:23:15PM +0100, Andrea Bolognani wrote:
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.
That's good to know. If developer tooling usage is being harmed that significantly strengthens the case for removing them. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 3/9/21 1:23 PM, Andrea Bolognani wrote:
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.
Yep. I've never seen the point either (especially after we realized the problem with const pointers several years ago); I just followed along blindly because that was the convention and I figured *someone* must have a special place in their heart for them. I wouldn't bat an eye if they were removed.
On a Tuesday in 2021, 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.
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.
Optimizing for reading speed is more important. So one more reason to get rid of this "obfuscation". But even for writing, I've had trouble getting coccinelle to understand some 'Ptr's, so I had to resort to slower alternatives. Jano
On Tue, Mar 09, 2021 at 17:44:16 +0000, Daniel P. Berrangé wrote: ...
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 ?
Oh yes, please go ahead and remove them. I learnt to use them as part of libvirt coding style when I started working on libvirt, but never really understood the reason behind them. Jirka
On Tue, Mar 09, 2021 at 17:44:16 +0000, Daniel Berrange 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;
[...]
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 ?
I don't usually declare them for internal types any more. I will not mourn if we discourage them from now on. I'd just prefer if we either big-bang rewrite all of it or just discourage it's use in new code and let it gradually die, but not encourage any semi-concetrated effort to remove the use of them, since that creates a stream of steady and long enduring churn. Basically don't make a GSoC project or file an public 'bite-sized' task for it.
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. Michal
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. Michal
On Wed, Mar 10, 2021 at 18:41:35 +0100, Michal Privoznik wrote:
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.
Yup, we could possibly automatically fix the variable declarations to be one-per-line at the beginning.
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);
This is weird ...
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.
.. so this is the right solution for that case.
On Wed, Mar 10, 2021 at 06:41:35PM +0100, Michal Privoznik wrote:
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.
Yes, we should be using g_autoptr even today, because g_auto is not for this use case: https://developer.gnome.org/glib/2.64/glib-Miscellaneous-Macros.html#g-auto "This is meant to be used with stack-allocated structures and non-pointer types. For the (more commonly used) pointer version, see g_autoptr()." Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (7)
-
Andrea Bolognani -
Daniel P. Berrangé -
Jiri Denemark -
Ján Tomko -
Laine Stump -
Michal Privoznik -
Peter Krempa