dbwrap_tool: Simplify persistent/non-persistent check / dbwrap_tool: Avoid an unnecessary "else"
Swen Schillig
swen at vnet.ibm.com
Tue Sep 4 07:10:19 UTC 2018
On Mon, 2018-09-03 at 16:10 -0700, 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.
I guess what's a bit distracting is the fact
that a logical result is gathered by an addition.
Would an XOR not be more clear here ?
if (persistent ^ non_persistent) {
...
I know that this is a only a bit-operation whereas a logical one
would be the 100% correct one here, but the current solution seems to
only support a boolean value-set as well.
Cheers Swen
More information about the samba-technical
mailing list