Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Dec 2022 13:14:48 +0800
From:   Vernon Yang <vernon2gm@gmail.com>
To:     Liam Howlett <liam.howlett@oracle.com>
Cc:     "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 6/8] maple_tree: change return type of mas_commit_b_node()
Message-ID: <Y6KWSLm2AqpqqfW7@vernon-pc>
References: <20221220142606.1698836-2-vernon2gm@gmail.com>
 <20221220142606.1698836-7-vernon2gm@gmail.com>
 <20221220150310.kfzx5paymtaepbxk@revolver>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221220150310.kfzx5paymtaepbxk@revolver>
Precedence: bulk

On Tue, Dec 20, 2022 at 03:03:36PM +0000, Liam Howlett wrote:
> * Vernon Yang <vernon2gm@gmail.com> [221220 09:26]:
> > The return value of mas_commit_b_node() function represents whether
> > the submit b_node is successful, and can only be false and true, so
> > the return value type is bool is more appropriate and for better
> > readability.
> >
> > Signed-off-by: Vernon Yang <vernon2gm@gmail.com>
> > ---
> >  lib/maple_tree.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> > index b3a215dd961e..e7dde4a1d6cb 100644
> > --- a/lib/maple_tree.c
> > +++ b/lib/maple_tree.c
> > @@ -3578,7 +3578,7 @@ static inline bool mas_reuse_node(struct ma_wr_state *wr_mas,
> >   * @b_node: The maple big node
> >   * @end: The end of the data.
> >   */
> > -static inline int mas_commit_b_node(struct ma_wr_state *wr_mas,
> > +static inline bool mas_commit_b_node(struct ma_wr_state *wr_mas,
> >  			    struct maple_big_node *b_node, unsigned char end)
>
> mas_commit_b_node() can also return the ints from mas_split() and
> mas_rebalance().  I'm fine with changing the return, but it seems odd to
> only half-change it?
Oh, sorry, I forgot to changes the return value type of the mas_split()
and mas_rebalance()

>
> Initially I had a different intention for the return type of int, but it
> seems the return isn't used at all from this function.  We should
> just change mas_commit_b_node(), mas_split(), and mas_rebalance() to
> return type void if we're going to clean it up.
Yes, the return isn't used at all from this function, have noticed.

Initially, I also wanted to change the return type void, but these
functions have an error condition that returns early, so I'm more
inclined to have an error value return so that it's clearer and
for better readable.

en... I temporarily remove this patch 6

>
> >  {
> >  	struct maple_node *node;
> > @@ -3598,7 +3598,7 @@ static inline int mas_commit_b_node(struct ma_wr_state *wr_mas,
> >
> >  	mas_node_count(wr_mas->mas, 1);
> >  	if (mas_is_err(wr_mas->mas))
> > -		return 0;
> > +		return false;
> >
> >  	node = mas_pop_node(wr_mas->mas);
> >  	node->parent = mas_mn(wr_mas->mas)->parent;
> > @@ -3607,7 +3607,7 @@ static inline int mas_commit_b_node(struct ma_wr_state *wr_mas,
> >  	mas_replace(wr_mas->mas, false);
> >  reuse_node:
> >  	mas_update_gap(wr_mas->mas);
> > -	return 1;
> > +	return true;
> >  }
> >
> >  /*
> > --
> > 2.34.1
> >
> >