Re: [PATCH v2 15/19] locking/lockdep: Remove __cq_empty()

[Bart Van Assche <bvanassche@acm.org>]

On Mon, 2019-03-18 at 16:57 +0800, Yuyang Du wrote:
> __cq_empty() can be embeded in __cq_dequeue(), removing it. We get slightly
> simpler code. No functional change.

Does inlining __cq_empty() really improve readability of the lockdep code?

> -static inline int __cq_dequeue(struct circular_queue *cq, struct lock_list **elem)
> +/*
> + * Dequeue an element from the circular_queue, return the lock if the queue
> + * is not empty, or NULL if otherwise
> + */
> +static inline struct lock_list * __cq_dequeue(struct circular_queue *cq)
>  {
> -       if (__cq_empty(cq))
> -               return -1;
> +       struct lock_list * lock;
>  
> -       *elem = cq->element[cq->front];
> +       /*
> +        * Is the circular_queue empty?
> +        */
> +       if (cq->front == cq->rear)
> +               return NULL;
> +
> +       lock = cq->element[cq->front];
>         cq->front = (cq->front + 1) & CQ_MASK;
> -       return 0;
> +
> +       return lock;
>  }
>  
>  static inline unsigned int  __cq_get_elem_count(struct circular_queue *cq)
> @@ -1376,6 +1381,7 @@ static int __bfs(struct lock_list *source_entry,
>                  int forward)
>  {
>         struct lock_list *entry;
> +       struct lock_list *lock;
>         struct list_head *head;
>         struct circular_queue *cq = &lock_cq;
>         int ret = 1;
> @@ -1397,10 +1403,7 @@ static int __bfs(struct lock_list *source_entry,
>         __cq_init(cq);
>         __cq_enqueue(cq, source_entry);
>  
> -       while (!__cq_empty(cq)) {
> -               struct lock_list *lock;
> -
> -               __cq_dequeue(cq, &lock);
> +       while ((lock = __cq_dequeue(cq))) {
>  
>                 if (!lock->class) {
>                         ret = -2;

This is the most important change in this patch. Using the title "Remove __cq_empty()"
for this patch is misleading because the above patch does something else, namely changing
the return type of __cq_dequeue() from int into a pointer. Should this patch perhaps be
split or should the __cq_empty() removal be left out from this patch?

Bart.