Re: [PATCH v3 13/13] migration/rdma: Remove all "ret" variables that are used only once

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Change code that is:
>>
>> int ret;
>> ...
>>
>> ret = foo();
>> if (ret[ < 0]?) {
>>
>> to:
>>
>> if (foo()[ < 0]) {
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/rdma.c | 29 ++++++++---------------------
>>  1 file changed, 8 insertions(+), 21 deletions(-)
>>
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index a43527a83c..c382588b26 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -1107,7 +1107,6 @@ err_alloc_pd_cq:
>>  static int qemu_rdma_alloc_qp(RDMAContext *rdma)
>>  {
>>      struct ibv_qp_init_attr attr = { 0 };
>> -    int ret;
>>  
>>      attr.cap.max_send_wr = RDMA_SIGNALED_SEND_MAX;
>>      attr.cap.max_recv_wr = 3;
>> @@ -1117,8 +1116,7 @@ static int qemu_rdma_alloc_qp(RDMAContext *rdma)
>>      attr.recv_cq = rdma->recv_cq;
>>      attr.qp_type = IBV_QPT_RC;
>>  
>> -    ret = rdma_create_qp(rdma->cm_id, rdma->pd, &attr);
>> -    if (ret < 0) {
>> +    if (rdma_create_qp(rdma->cm_id, rdma->pd, &attr) < 0) {
>
> This particular pattern hurts readability IMO. See how the < 0 got
> pushed all the way to the end of the line. The longer the list of
> arguments, the larger the chance of missing the < 0 when glancing over
> the code.

You can't have both:

* No "ret" variable that is used only once.
* make the condition line small

I choosed the 1st one.  For "ret" I always asume that you set it in one
place and you use it in multiple places.  Otherwise you don't need it.

But I can understand the other point of view.

There is a reason that this patch was last O:-)

> Anyway:
>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>

Thanks.