[libvirt PATCH 05/14] Remove pointless initializations

Ján Tomko posted 14 patches 5 years, 4 months ago
There is a newer version of this series
[libvirt PATCH 05/14] Remove pointless initializations
Posted by Ján Tomko 5 years, 4 months ago
Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
 src/test/test_driver.c | 2 +-
 tests/virnumamock.c    | 2 +-
 tests/virrandommock.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index d582c207f4..cbbfea6665 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4354,7 +4354,7 @@ testNodeGetFreePages(virConnectPtr conn G_GNUC_UNUSED,
                      unsigned long long *counts,
                      unsigned int flags)
 {
-    size_t i = 0, j = 0;
+    size_t i, j;
     int x = 6;
 
     virCheckFlags(0, -1);
diff --git a/tests/virnumamock.c b/tests/virnumamock.c
index 40e18e646e..d39c264a3f 100644
--- a/tests/virnumamock.c
+++ b/tests/virnumamock.c
@@ -130,7 +130,7 @@ virNumaGetPages(int node,
 {
     const int pages_def[] = { 4, 2 * 1024, 1 * 1024 * 1024};
     const int npages_def = G_N_ELEMENTS(pages_def);
-    size_t i = 0;
+    size_t i;
 
     if (pages_size)
         *pages_size = g_new0(unsigned int, npages_def);
diff --git a/tests/virrandommock.c b/tests/virrandommock.c
index 6dd15213e3..ca0520a5a3 100644
--- a/tests/virrandommock.c
+++ b/tests/virrandommock.c
@@ -69,7 +69,7 @@ int
 gnutls_dh_params_generate2(gnutls_dh_params_t dparams,
                            unsigned int bits)
 {
-    int rc = 0;
+    int rc;
 
     VIR_MOCK_REAL_INIT(gnutls_dh_params_generate2);
 
-- 
2.26.2

Re: [libvirt PATCH 05/14] Remove pointless initializations
Posted by Martin Kletzander 5 years, 4 months ago
On Wed, Sep 23, 2020 at 08:14:54PM +0200, Ján Tomko wrote:
>Signed-off-by: Ján Tomko <jtomko@redhat.com>
>---
> src/test/test_driver.c | 2 +-
> tests/virnumamock.c    | 2 +-
> tests/virrandommock.c  | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>

As mentioned in the previous patch I do not think they are pointless in the long
run.  I prefer ret, rc, rv and similar to be initialised to error/negative value
and others to their null values (numbers to zero, pointers to NULL etc.) even
though I know this is a very subjective opinion.

If you really want to push for this then it would require making

Of course the compiler can check if the value is used before initialisation.
And I hope all current compilers do that with our warning options.  Except they
cannot check for that if you only pass the address of the value.  I guess these
are fine, but I would not like to create a precedent.

What I think we should do instead is make an exception for defining variables
for iteration in the for loop itself.  I know it's only 21 years since it was
standardised and we want to make sure all the compilers and distros can handle
it... /s

OK, I understand that defining variables in the middle of the function most of
the time clutters the code, but for 'i' and 'j' I, for one, see simply no reason
to disallow that.

>diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>index d582c207f4..cbbfea6665 100644
>--- a/src/test/test_driver.c
>+++ b/src/test/test_driver.c
>@@ -4354,7 +4354,7 @@ testNodeGetFreePages(virConnectPtr conn G_GNUC_UNUSED,
>                      unsigned long long *counts,
>                      unsigned int flags)
> {
>-    size_t i = 0, j = 0;
>+    size_t i, j;
>     int x = 6;
>
>     virCheckFlags(0, -1);
>diff --git a/tests/virnumamock.c b/tests/virnumamock.c
>index 40e18e646e..d39c264a3f 100644
>--- a/tests/virnumamock.c
>+++ b/tests/virnumamock.c
>@@ -130,7 +130,7 @@ virNumaGetPages(int node,
> {
>     const int pages_def[] = { 4, 2 * 1024, 1 * 1024 * 1024};
>     const int npages_def = G_N_ELEMENTS(pages_def);
>-    size_t i = 0;
>+    size_t i;
>
>     if (pages_size)
>         *pages_size = g_new0(unsigned int, npages_def);
>diff --git a/tests/virrandommock.c b/tests/virrandommock.c
>index 6dd15213e3..ca0520a5a3 100644
>--- a/tests/virrandommock.c
>+++ b/tests/virrandommock.c
>@@ -69,7 +69,7 @@ int
> gnutls_dh_params_generate2(gnutls_dh_params_t dparams,
>                            unsigned int bits)
> {
>-    int rc = 0;
>+    int rc;
>
>     VIR_MOCK_REAL_INIT(gnutls_dh_params_generate2);
>
>-- 
>2.26.2
>
Re: [libvirt PATCH 05/14] Remove pointless initializations
Posted by Daniel P. Berrangé 5 years, 4 months ago
On Thu, Sep 24, 2020 at 10:30:21AM +0200, Martin Kletzander wrote:
> On Wed, Sep 23, 2020 at 08:14:54PM +0200, Ján Tomko wrote:
> > Signed-off-by: Ján Tomko <jtomko@redhat.com>
> > ---
> > src/test/test_driver.c | 2 +-
> > tests/virnumamock.c    | 2 +-
> > tests/virrandommock.c  | 2 +-
> > 3 files changed, 3 insertions(+), 3 deletions(-)
> > 
> 
> As mentioned in the previous patch I do not think they are pointless in the long
> run.  I prefer ret, rc, rv and similar to be initialised to error/negative value
> and others to their null values (numbers to zero, pointers to NULL etc.) even
> though I know this is a very subjective opinion.

Yeah, I tend to agree. From a defensive coding POV it is preferrable to
initialize every single variable at time of declaration to a known value.

While compilers and coverity can check for use of uninitialized variables,
I don't think their checks have ever been perfect, because new versions of
compilers tend to discover things periodically.


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