[SCM] Samba Shared Repository - branch master updated

[Matthias Dieter Wallnöfer]

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