[SCM] Samba Shared Repository - branch master updated

[Jelmer Vernooij]

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.

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/f1a7610b/attachment.pgp>