Re: [libvirt] [PATCHv2 1/1] include: function parameter names same in declaration

[Daniel P . Berrangé]

On Tue, Feb 13, 2018 at 01:56:59PM -0500, Christopher Venteicher wrote:
> 
> 
> - Original Message -
> > From: "Bjoern Walk" <bw...@linux.vnet.ibm.com>
> > To: "Daniel P. Berrangé" <berra...@redhat.com>
> > Cc: "Chris Venteicher" <cvent...@redhat.com>, libvir-list@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é <berra...@redhat.com> [2018-02-13, 09:47AM +]:
> > > 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,
> > > 
> > > H, 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.

Ultimately this is all just code churn for no functional benefit. So if we
are going to go through this churn, I'd like to see the benefit maximised.
To me having consistent name for each common data type is more important,
so I'd like to see that be the focus, rather than just blindly applying
the results of clang-tidy.


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 1/1] include: function parameter names same in declaration

[Daniel P . Berrangé]

On Tue, Feb 13, 2018 at 12:03:40PM +0100, Bjoern Walk wrote:
> Daniel P. Berrangé  [2018-02-13, 09:47AM +]:
> > 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,
> > 
> > H, 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'.

That is a situation we should try to avoid as remembering which type
is which for dom vs domain is painful. We generally want to aim for

  virDomainPtr  dom
  virDomainObjPtr obj or vm
  
> 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.

"domain" is unneccessarily verbose.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 1/1] include: function parameter names same in declaration

[Christopher Venteicher]

- Original Message -
> From: "Bjoern Walk" <bw...@linux.vnet.ibm.com>
> To: "Daniel P. Berrangé" <berra...@redhat.com>
> Cc: "Chris Venteicher" <cvent...@redhat.com>, libvir-list@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é <berra...@redhat.com> [2018-02-13, 09:47AM +]:
> > 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,
> > 
> > H, 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

Re: [libvirt] [PATCHv2 1/1] include: function parameter names same in declaration

[Bjoern Walk]

Daniel P. Berrangé  [2018-02-13, 09:47AM +]:
> 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,
> 
> H, 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.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 1/1] include: function parameter names same in declaration

[Daniel P . Berrangé]

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,

H, 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

I would have a marginal preference for us to bring the the header in line
with the source code as a whole and pick "dom". Any one else have opinions.

Also, if doing this, we should add a cfg.mk syntax-check rule for it.


>   const char *device,
>   
> virTypedParameterPtr params,
>   int nparams, 
> unsigned int flags);
> -int virDomainGetInterfaceParameters (virDomainPtr dom,
> +int virDomainGetInterfaceParameters (virDomainPtr domain,
>   const char *device,
>   
> virTypedParameterPtr params,
>   int *nparams, 
> unsigned int flags);
> @@ -1697,7 +1697,7 @@ struct _virDomainBlockInfo {
>  * offset, similar to 'ls')*/
>  };
>  
> -int virDomainGetBlockInfo(virDomainPtr dom,
> +int virDomainGetBlockInfo(virDomainPtr domain,
>const char *disk,
>virDomainBlockInfoPtr info,
>unsigned int flags);
> @@ -1869,7 +1869,7 @@ int virDomainPinEmulator   
> (virDomainPtr domain,
>  unsigned int flags);
>  
>  int virDomainGetEmulatorPinInfo (virDomainPtr domain,
> - unsigned char *cpumaps,
> + unsigned char *cpumap,
>   int maplen,
>   unsigned int flags);
>  
> @@ -2298,11 +2298,11 @@ void 
> virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats);
>   */
>  # define VIR_PERF_PARAM_EMULATION_FAULTS  "emulation_faults"
>  
> -int virDomainGetPerfEvents(virDomainPtr dom,
> +int virDomainGetPerfEvents(virDomainPtr domain,
> virTypedParameterPtr *params,
> int *nparams,
> unsigned int flags);
> -int virDomainSetPerfEvents(virDomainPtr dom,
> +int virDomainSetPerfEvents(virDomainPtr domain,
> virTypedParameterPtr params,
> int nparams,
> unsigned int flags);
> @@ -3130,14 +3130,14 @@ typedef enum {
>* completed job */
>  } virDomainGetJobStatsFlags;
>  
> -int virDomainGetJobInfo(virDomainPtr dom,
> +int virDomainGetJobInfo(virDomainPtr domain,
>  virDomainJobInfoPtr info);
>  int virDomainGetJobStats(virDomainPtr domain,
>   int *type,
>   virTypedParameterPtr *params,
>   int *nparams,
>   unsigned int flags);
> -int virDomainAbortJob(virDomainPtr dom);
> +int virDomainAbortJob(virDomainPtr domain);
>  
>  typedef enum {
>  VIR_DOMAIN_JOB_OPERATION_UNKNOWN = 0,
> diff --git a/include/libvirt/libvirt-event.h b/include/libvirt/libvirt-event.h
> index 23227d090..0293a2841 100644
> --- a/include/libvirt/libvirt-event.h
> +++ b/include/libvirt/libvirt-event.h
> @@ -179,11 +179,11 @@ int virEventAddHandle(int fd, int events,
>  void virEventUpdateHandle(int watch, int events);
>  int virEventRemoveHandle(int watch);
>  
> -int virEventAddTimeout(int frequency,
> +int virEventAddTimeout(int timeout,
>