On Mon, Mar 11, 2019 at 05:31:05AM -0700, Matthew Wilcox wrote:
> On Mon, Mar 11, 2019 at 12:13:54PM +0000, Jason Gunthorpe wrote:
> > > @@ -1059,14 +1050,14 @@ static int assign_client_id(struct ib_client *client)
> > > * to get the LIFO order. The extra linked list can go away if xarray
> > > * learns to reverse iterate.
> > > */
> > > - if (list_empty(&client_list))
> > > + if (list_empty(&client_list)) {
> > > client->client_id = 0;
> > > - else
> > > - client->client_id =
> > > - list_last_entry(&client_list, struct ib_client, list)
> > > - ->client_id;
> > > - ret = xa_alloc(&clients, &client->client_id, INT_MAX, client,
> > > - GFP_KERNEL);
> > > + } else {
> > > + struct ib_client *last = list_last_entry(&client_list,
> > > + struct ib_client, list);
> > > + client->client_id = last->client_id + 1;
> >
> > blank line after locals, but other wise these all looks fine..
>
> Would you rather see this rendered as:
>
> if (list_empty(&client_list)) {
> client->client_id = 0;
> } else {
> struct ib_client *last;
>
> last = list_last_entry(&client_list, struct ib_client, list);
> client->client_id = last->client_id + 1;
> }
Don't care much either way. Only that the Linux style guide is to
always have the blank line after variable declarations in any block
> or move the declaration of 'last' up to the top of the function?
This one I dislike, variables should be in their narrowest scope for
clarity
> > Should have started out with the xa_insert version above..
>
> I didn't spot it until last night either ...
It is a leftover weird thinking logic from an earlier attempt I had
that was trying to get the last ID out of the xarray without the
linked list..
Jason
