[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 05/21] util: Rename virBitmapDataToString to virBitmapDataFormat




On 11/13/2017 03:50 AM, Martin Kletzander wrote:
> It is literally only a wrapper around virBitmapNewData() and
> virBitmapFormat(), only the naming was wrong since it was introduced.

in commit id '7d8afc47'.

> And because we have virBitmap*String functions where the meaning of
> the 'String' is constant, this might confuse someone.

And some would say DataFormatWhat - cannot please everyone though ;-)
because "naming is hard" (per abologna).

FWIW: When you noted the 'String' is constant, I immediately thought
about a "const char *String" meaning it wouldn't need to be free'd.

> 
> Signed-off-by: Martin Kletzander <mkletzan redhat com>
> ---
>  src/libvirt_private.syms | 2 +-
>  src/util/virbitmap.c     | 6 +++---
>  src/util/virbitmap.h     | 4 ++--
>  tests/virbitmaptest.c    | 4 ++--
>  tools/virsh-domain.c     | 4 ++--
>  tools/virsh-host.c       | 2 +-
>  6 files changed, 11 insertions(+), 11 deletions(-)
> 

So like patch 04/21 - patch 06/21 also hasn't shown up yet, so please
consider the following for comments and R-b (all the others showed up).

1. The commit message needs massaging - "fromvirBitmapToString" to be
"from virBitmapToString"

2. The 3rd argument should be on it's only line (like noted in 03)

With the slight adjustment below, consider both 05 and 06 as:

Reviewed-by: John Ferlan <jferlan redhat com>

John

[...]

> diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
> index c24d4a72f70a..488796719dd9 100644
> --- a/tests/virbitmaptest.c
> +++ b/tests/virbitmaptest.c

There's a comment above here that needs adjustment ...

s/DataToString/DataFormat

to be consistent...

> @@ -336,12 +336,12 @@ test5(const void *v ATTRIBUTE_UNUSED)
>          data2[4] != 0x04)
>          goto error;
>  
> -    if (!(str = virBitmapDataToString(data, sizeof(data))))
> +    if (!(str = virBitmapDataFormat(data, sizeof(data))))
>          goto error;
>      if (STRNEQ(str, "0,9,34"))
>          goto error;
>      VIR_FREE(str);
> -    if (!(str = virBitmapDataToString(data2, len2)))
> +    if (!(str = virBitmapDataFormat(data2, len2)))
>          goto error;
>      if (STRNEQ(str, "0,2,9,15,34"))
>          goto error;
[...]


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]