| Message ID | 20200813153614.39917-1-liq3ea@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw: virtio-gpu: remove duplicated 'virtqueue_pop' | expand |
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
Laszlo Ersek <lersek@redhat.com> 于2020年8月14日周五 上午4:21写道： > > 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. > I have no strong opinion about the style(2) and style(3), just don't like the dup of style(1). Anyway, let Gerd do the choice. AFAICS, the qemu uses a lot of stype(2) to populate virtio requests. Thanks, Li Qiang > Just my opinion of course; I don't feel too strongly about this. > > Laszlo >
Hi, > - If the get_next() invocation is simple, then style (1) is perfectly fine. Fully agree. Duplicating a single line is perfectly fine if it is simple enough that you can hardly get it wrong. > - Style (2) is the worst of all. Yes, especially because the break is three lines not two due to qemu code style. > - 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: > or more verbosely > > while ((cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command))) != > NULL) { Style 3 tends to generate long lines, which in turn need line breaks like this. Also many parens don't help making code more readable. So I'd prefer to just leave things as-is. take care, Gerd
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);
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(-)