Re: [libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser

On Fri, Oct 01, 2021 at 01:13:00PM -0400, Laine Stump wrote: > On 10/1/21 5:57 AM, Daniel P. Berrangé wrote: > > On Mon, Sep 27, 2021 at 10:30:47PM +0300, Dmitrii Shcherbakov wrote: > > [...] > > > +GType > > > +vir_pci_vpd_resource_type_get_type(void) > > I know you had asked about using under_scored_naming in a reply to Peter > pointing out "non-standard" names in V3 of your patches, but I don't think > anyone followed up on that. That very specific case is something that is required when we use GObject for the type. see util/viridentity.h for the same scenario. There are a couple of other similar requirements force on us by GObject in this regard, but aside from that we should follow normal libvirt naming practice.

> > > +static gboolean > > In a similar "coding standards" vein - other uses of "gboolean" (rather than > the much more common "bool", or *very rarely* "boolean") in our code seem to > be the product of auto-generated(?) xdr headers for wireshark, functions > that are registered as callbacks to glib (and so must follow the types in > the callback function pointer prototype), and a very small number of other > cases. Do we want new code using gboolean rather than bool in these "other" > cases that don't require gboolean for proper glib type compatibility? gboolean is a completely differnt sized type from bool. As a general rule we want to use 'bool' + true/false, except where we must have strict type compatibility with glib. The main scenario that matters is callbacks integrating with GLib's event loop or similar. All the other gNNNN basic types are essentially identical to stdint.h types, so there's no compelling reason to use them. We can use the plain C types, or the stdint.h sized types as appropriate and they'll be fully compatible with any GLib APIs. > (a side-nit completely unrelated to your patches - I noticed that in at > least a couple places in libvirt, a gboolean is assigned "true" or "false", > although the glib documentation says that it should only have the value > "TRUE" or "FALSE". Good thing those happen to use the same values!) We're lucky in that the constants do the right thing if you assign/compare. eg a bool foo= TRUE; if (foo == TRUE) .. will be ok. I'm not so sure about bool foo = TRUE gboolean bar == TRUE; if (foo == bar) ... because they're different types, and so when you size-extend I think they end up with different values, despite both being TRUE.