Hi Jelmer,
Jelmer Vernooij wrote:
Hi Matthias,
On Mon, 2010-11-29 at 19:20 +0100, Matthias Dieter Wallnöfer wrote:
I don't see me very guilty since I've simply performed the cleanup as
the Solaris "cc" suggests.
Just because Solaris CC gives a warning does not mean that whatever it
suggests is the right thing to do. You are the person making these
changes changes and pushing them into master, so you're responsible for
them.
Yes, I am and I know this.
I see more a problem in the way how these tests were written - since
obviously there wasn't enough care to proof for the termination
conditions - which should always be taken into account.
Therefore I think it doesn't make much sense to revert my commit - since
functionally it doesn't change anything - they are wrong with and
without my commit.
Much better would be to immediately introduce the termination conditions.
This is a boolean function. All code paths should in principal return a
boolean. If we expect a code path to never be used we should add a
comment saying so and return false.
Alternatively (and I'd prefer that), in cases like these, we should
"break;" in the endless loop rather than returning directly from that
loop to make the code easier to read.
Okay - this is a good point of view - and personally I would also prefer
this, but also the other style is acceptable (or do we have a coding
guideline regarding this?).
Have you checked that none of the compilers we support warns about a
missing return statement in these cases?
No one warns - as far as I've seen. And I've checked these changes
carefully - and "cc" was always correct.
What exactly is the improvement of this change over the previous
situation ?
I think this depends on the compiler. On a part probably nothing and on
others the "return" simply doesn't get compiled into the binary - so a
small file size improvement.
Cheers,
Matthias
Cheers,
Jelmer
Jelmer Vernooij wrote:
Hi Matthias,
On Mon, 2010-11-29 at 15:35 +0100, Matthias Dieter Wallnöfer wrote:
diff --git a/source4/torture/basic/base.c b/source4/torture/basic/base.c
index d5090e9..9953573 100644
--- a/source4/torture/basic/base.c
+++ b/source4/torture/basic/base.c
@@ -1360,8 +1360,6 @@ static bool run_iometer(struct torture_context *tctx,
smbcli_errstr(cli->tree)));
}
}
-
- return true;
}
This is wrong; this function should return a bool and it should always
return true in case of success. Please revert it.
/**
diff --git a/source4/torture/basic/misc.c b/source4/torture/basic/misc.c
index 7223272..c590237 100644
--- a/source4/torture/basic/misc.c
+++ b/source4/torture/basic/misc.c
@@ -289,8 +289,6 @@ bool torture_holdopen(struct torture_context *tctx,
fflush(stdout);
sleep(15);
}
-
- return true;
}
Same here.
/*
diff --git a/source4/torture/raw/pingpong.c b/source4/torture/raw/pingpong.c
index 124cf69..f9c551e 100644
--- a/source4/torture/raw/pingpong.c
+++ b/source4/torture/raw/pingpong.c
@@ -243,8 +243,5 @@ bool torture_ping_pong(struct torture_context *torture)
}
loops++;
}
-
- talloc_free(mem_ctx);
- return true;
}
Same here.
FWIW I've only checked these three fragments since they're in code I'm
familiar with. I haven't checked the other ones.
Can you, per tridge's recent request, please send your patches for
review before checking them in?
Cheers,
Jelmer
