12:56 p.m.
----- Original Message -----
From: "Bjoern Walk" <bwalk(a)linux.vnet.ibm.com>
To: "Daniel P. Berrangé" <berrange(a)redhat.com>
Cc: "Chris Venteicher" <cventeic(a)redhat.com>, libvir-list(a)redhat.com
Sent: Tuesday, February 13, 2018 5:03:40 AM
Subject: Re: [libvirt] [PATCHv2 1/1] include: function parameter names same in
declaration
Daniel P. Berrangé <berrange(a)redhat.com> [2018-02-13, 09:47AM +0000]:
> On Mon, Feb 12, 2018 at 03:24:03PM -0600, Chris Venteicher wrote:
> > Headers use same function parameter names as definition code.
> >
> > In some cases in libvirt-domain and libvirt-network an
> > established
> > naming pattern in the header files was more consistent and
> > informative
> > in which case the implementation was modified in the c file.
>
> > @@ -1626,11 +1626,11 @@ int
> > virDomainInterfaceStats (virDomainPtr dom,
> > */
> > # define VIR_DOMAIN_BANDWIDTH_OUT_BURST "outbound.burst"
> >
> > -int virDomainSetInterfaceParameters
> > (virDomainPtr dom,
> > +int virDomainSetInterfaceParameters
> > (virDomainPtr domain,
>
> Hmmmm, I kind of expected that "dom" would be more popular than
> "domain",
> but I see the results are somewhat contradictory.
>
> If we just consider the header file
>
> $ git grep "virDomainPtr dom" include/libvirt/libvirt-domain.h |
> wc -l
> 167
> $ git grep "virDomainPtr dom" include/libvirt/libvirt-domain.h |
> grep "virDomainPtr domain" | wc -l
> 99
>
> So dom==68, domain=99 => 2:3
>
> But if we consider the source as a whole
>
> $ git grep "virDomainPtr dom" | wc -l
> 1863
> $ git grep "virDomainPtr dom" | grep "virDomainPtr domain" | wc
-l
> 675
>
> So dom=1188 domain=675 => 2:1
This is biased because for a temporary variable an abbreviated name
'dom' is preferable, especially if you have an argument 'domain'.
Since function declarations serve some kind of documentation purpose,
I'd prefer the full name 'domain'. It reads so much better in my
opinion
and characters are cheap.
A little more background ...
I have only submitted the changes for include/libvirt/*.h so far.
At the bottom is a list of the files impacted by a total of 286 changes suggested by the
clang-tidy filter.
The focus here is only the horizontal relationship establishing consistency between
definition and declaration,
not the vertical line relationship of consistency between function definitions.
function_definition_1(p1, p2, p3) ---> function_declaration_1(p1,p2,p3)
|
|
|
function_definition_2(p1, p2, p3) ---> function_declaration_2(p1,p2,p3)
|
|
|
function_definition_n(p1, p2, p3) ---> function_declaration_n(p1,p2,p3)
My guess is > 90 percent of the 286 changes make the declaration better.
Probably < 10 percent of the 286 changes make the declaration slightly less readable
(ex. domain->dom).
All of the changes make the declarations consistent with the definitions.
None of the changes make the function_definitions worse.
Chris
daemon/remote.c | 2 +-
daemon/stream.h | 2 +-
include/libvirt/libvirt-domain.h | 26 ++++++++++++------------
include/libvirt/libvirt-event.h | 4 ++--
include/libvirt/libvirt-host.h | 4 ++--
include/libvirt/libvirt-interface.h | 4 ++--
include/libvirt/libvirt-network.h | 6 +++---
include/libvirt/libvirt-nwfilter.h | 2 +-
include/libvirt/libvirt-qemu.h | 2 +-
include/libvirt/libvirt-secret.h | 4 ++--
include/libvirt/libvirt-storage.h | 12 ++++++------
include/libvirt/libvirt-stream.h | 22 ++++++++++-----------
src/access/viraccessmanager.c | 2 +-
src/access/viraccessmanager.h | 4 ++--
src/conf/capabilities.c | 2 +-
src/conf/capabilities.h | 2 +-
src/conf/cpu_conf.h | 6 +++---
src/conf/domain_audit.h | 12 ++++++------
src/conf/domain_event.h | 4 ++--
src/conf/networkcommon_conf.h | 4 ++--
src/conf/numa_conf.h | 4 ++--
src/conf/nwfilter_conf.h | 2 +-
src/conf/nwfilter_params.h | 10 +++++-----
src/conf/object_event_private.h | 2 +-
src/conf/secret_conf.h | 2 +-
src/conf/snapshot_conf.h | 4 ++--
src/conf/storage_conf.h | 4 ++--
src/conf/virdomainobjlist.h | 4 ++--
src/conf/virinterfaceobj.c | 2 +-
src/conf/virnetworkobj.c | 4 ++--
src/conf/virnetworkobj.h | 18 ++++++++---------
src/conf/virnodedeviceobj.c | 2 +-
src/conf/virnodedeviceobj.h | 4 ++--
src/conf/virsecretobj.c | 2 +-
src/datatypes.h | 8 ++++----
src/driver.h | 2 +-
src/libvirt_internal.h | 26 ++++++++++++------------
src/locking/lock_manager.h | 10 +++++-----
src/logging/log_handler.h | 2 +-
src/lxc/lxc_monitor.c | 2 +-
src/node_device/node_device_driver.h | 12 ++++++------
src/nwfilter/nwfilter_gentech_driver.h | 4 ++--
src/openvz/openvz_driver.c | 2 +-
src/openvz/openvz_util.h | 2 +-
src/qemu/qemu_agent.h | 6 +++---
src/qemu/qemu_alias.h | 6 +++---
src/qemu/qemu_block.h | 2 +-
src/qemu/qemu_capabilities.h | 4 ++--
src/qemu/qemu_capspriv.h | 2 +-
src/qemu/qemu_cgroup.h | 2 +-
src/qemu/qemu_conf.h | 2 +-
src/qemu/qemu_hotplug.h | 6 +++---
src/qemu/qemu_monitor.h | 8 ++++----
src/qemu/qemu_process.h | 4 ++--
src/rpc/virnetmessage.h | 2 +-
src/rpc/virnetserver.h | 2 +-
src/rpc/virnetserverservice.h | 2 +-
src/rpc/virnetsocket.h | 16 +++++++--------
src/secret/secret_util.h | 4 ++--
src/security/security_dac.h | 2 +-
src/security/security_manager.h | 36 +++++++++++++++++-----------------
src/storage/storage_backend.h | 2 +-
src/uml/uml_conf.h | 2 +-
src/util/viralloc.h | 8 ++++----
src/util/virarch.h | 2 +-
src/util/viraudit.h | 2 +-
src/util/virbitmap.h | 2 +-
src/util/virbuffer.h | 4 ++--
src/util/vircommand.h | 4 ++--
src/util/virerror.h | 4 ++--
src/util/virfile.h | 16 +++++++--------
src/util/virhash.h | 2 +-
src/util/virhostdev.h | 20 +++++++++----------
src/util/viridentity.c | 2 +-
src/util/viriptables.h | 2 +-
src/util/virjson.h | 26 ++++++++++++------------
src/util/virkeycode.h | 2 +-
src/util/virkeyfile.h | 2 +-
src/util/virlog.h | 6 +++---
src/util/virmacaddr.h | 10 +++++-----
src/util/virnetdevbridge.h | 4 ++--
src/util/virnetdevmacvlan.h | 10 +++++-----
src/util/virnetdevvportprofile.h | 4 ++--
src/util/virobject.h | 16 +++++++--------
src/util/virpci.h | 2 +-
src/util/virpidfile.h | 6 +++---
src/util/virqemu.h | 2 +-
src/util/virresctrl.h | 4 ++--
src/util/virsexpr.h | 2 +-
src/util/virsocketaddr.h | 8 ++++----
src/util/virstring.h | 2 +-
src/util/virthreadjob.h | 2 +-
src/util/virthreadpool.h | 2 +-
src/util/virusb.h | 2 +-
src/util/viruuid.h | 4 ++--
src/vbox/vbox_snapshot_conf.h | 4 ++--
src/vmware/vmware_conf.h | 4 ++--
tests/testutils.h | 2 +-
tools/vsh.h | 8 ++++----
99 files changed, 286 insertions(+), 286 deletions(-)