[SCM] Samba Shared Repository - branch master updated

[Jelmer Vernooij]

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.

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

Have you checked that none of the compilers we support warns about a
missing return statement in these cases? 

What exactly is the improvement of this change over the previous
situation ?

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20101129/2c2ac939/attachment.pgp>