dbwrap_tool: Simplify persistent/non-persistent check / dbwrap_tool: Avoid an unnecessary "else"

jim jim.brown at rsmas.miami.edu
Mon Sep 3 23:37:22 UTC 2018


Add a comment before either version making clear the intent of the if 
statement:
"either one of persistent or non_persistent must be set to 1, but not 
both, or neither." (per Jeremy)

On 9/3/2018 7:10 PM, Jeremy Allison via samba-technical wrote:
> On Tue, Sep 04, 2018 at 09:43:32AM +1200, Andrew Bartlett via samba-technical wrote:
>> On Mon, 2018-09-03 at 23:30 +0200, Ralph Böhme wrote:
>>> Howdy,
>>>
>>> On Tue, Sep 04, 2018 at 09:11:32AM +1200, Andrew Bartlett wrote:
>>>> While each of these looks reasonable on its own, is this really the
>>>> clearest (rather than cleverest) possible way this could have been
>>>> constructed as a whole?
>>> s/clearest/clearer/
>>>
>>> yes. The clearest way? Who knows. Patches welcome.
>>>
>>>>> -       if ((persistent == 0 && non_persistent == 0) ||
>>>>> -           (persistent == 1 && non_persistent == 1))
>>>>> -       {
>>>>> +       if ((persistent + non_persistent) != 1) {
>>>>>                  d_fprintf(stderr, "ERROR: you must specify exactly one "
>>>>>                            "of --persistent and --non-persistent\n");
>>>>>                  goto done;
>>>>> -       } else if (non_persistent == 1) {
>>>>> +       }
>>>>> +       if (non_persistent == 1) {
>>>>>                  tdb_flags |= TDB_CLEAR_IF_FIRST;
>>>>>          }
>>>> What specifically was the goal of the change here?
>>> What specifically is the goal of this rhetorical question?
>> I was hoping to hear a rationale as to how this is 'simplified', to
>> argue against such 'simplification' in the future and to rhetorically
>> suggest that isn't the case here.
> It's a matter of personal taste. IMHO, the new
> code is a lot clearer to me.
>
> It's hard for me to see exactly what:
>
> 	if ((persistent == 0 && non_persistent == 0) ||
> 	    (persistent == 1 && non_persistent == 1))
>
> is trying to enforce, but:
>
> 	if ((persistent + non_persistent) != 1) {
>
> is *much* clearer (either one of persistent or
> non_persistent must be set to 1, but not both,
> or neither).
>
> I guess it just comes down to how your mind
> works when looking at these things.
>
>
>




More information about the samba-technical mailing list