[SCM] Samba Shared Repository - branch master updated

[Jelmer Vernooij]

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

-------------- 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-cvs/attachments/20101129/7b3096d2/attachment.pgp>