On Mon, 2010-11-29 at 20:26 +0100, Jelmer Vernooij wrote: > On Mon, 2010-11-29 at 19:56 +0100, Matthias Dieter Wallnöfer wrote: > > Jelmer Vernooij wrote: > > > 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. > Then please don't say you don't consider yourself responsible (perhaps > there is some language confusion here?). > > > >> 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?). > The coding style guide is important, but even more important is common > sense. Just because the coding style doesn't forbid something doesn't > mean it's a good idea. > > > > 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. > Most compilers probably wouldn't even compile this code into the binary > if they noticed it wasn't reachable. Even if they did then this > improvement is negligible. It will save us literally a couple of mmapped > bytes in the testsuite code at most. > > Please look at the bigger picture. These are a few bytes. A full Samba 4 > developer build is something like 230 Mb. Again, even if the compiler > didn't optimize out that return statement then there wouldn't be any > relevant difference. (I'll leave out fs blocks for simplicity here) > > If we were that desperate to reduce the size of our binaries on disk > then we would be reducing the length of the names of the symbols we > export, we'd be removing DEBUG statements, we'd be removing comments in > Python code. > > I could the point in changing this code if it made it more readable, but > it doesn't do that. That should of course read "I could *see* the point in changing this code if it made it more readable, but it doesn't do that."
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 > > >>> > > >>> > > >> > > > > > >
signature.asc
Description: This is a digitally signed message part