Re: [PATCH] hw: virtio-gpu: remove duplicated 'virtqueue_pop'

[Laszlo Ersek <lersek@redhat.com>]

On 08/13/20 17:36, Li Qiang wrote:
> Just use 'while (true)' to avoid duplicated.
> No function change.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  hw/display/virtio-gpu.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 5f0dd7c150..9cef313f5e 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -869,13 +869,15 @@ static void virtio_gpu_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>      }
>  #endif
>  
> -    cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
> -    while (cmd) {
> +    while (true) {
> +        cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
> +        if (!cmd) {
> +            break;
> +        }
>          cmd->vq = vq;
>          cmd->error = 0;
>          cmd->finished = false;
>          QTAILQ_INSERT_TAIL(&g->cmdq, cmd, next);
> -        cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
>      }
>  
>      virtio_gpu_process_cmdq(g);
> 

There are (at least) three styles:

(1)

    thing = get_next();
    while (is_valid(thing)) {
        ...
        thing = get_next();
    }

(2)

    while (true) {
        thing = get_next();
        if (!is_valid(thing)) {
            break;
        }
        ...
     }

(3)

    while (is_valid(thing = get_next())) {
        ...
    }

My opinion:

- If the get_next() invocation is simple, then style (1) is perfectly fine.

- Style (2) is the worst of all.

- If style (1) is not appropriate for whatever reason, then style (3) is frequently a good replacement. Style (3) is sometimes rejected by coding style documents though. Style (3) is not usable if is_valid() is a function-like macro that does not evaluate its argument exactly once. Frequently, is_valid() is simply open-coded with C operators (using extra parens), for example:

    while ((cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command)))) {

or more verbosely

    while ((cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command))) !=
           NULL) {

If we really dislike style (1), then I'd propose style (3). I think the present patch (style (2)) is a step back.

Just my opinion of course; I don't feel too strongly about this.

Laszlo