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:
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 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.