Re: [Tarantool-patches] [PATCH 1/3] wal: sanitize wal_mode

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

24.04.2021 00:17, Cyrill Gorcunov via Tarantool-patches пишет:
> On Fri, Apr 23, 2021 at 10:57:11PM +0200, Vladislav Shpilevoy wrote:
>> Hi! Thanks for the patch!
>>
>>> diff --git a/src/box/wal.h b/src/box/wal.h
>>> index a8a16ba2e..470aa1cc8 100644
>>> --- a/src/box/wal.h
>>> +++ b/src/box/wal.h
>>> @@ -41,7 +41,29 @@ struct fiber;
>>>   struct wal_writer;
>>>   struct tt_uuid;
>>>   
>>> -enum wal_mode { WAL_NONE = 0, WAL_WRITE, WAL_FSYNC, WAL_MODE_MAX };
>>> +enum wal_mode {
>>> +	/**
>>> +	 * Do not write data at all.
>>> +	 */
>>> +	WAL_NONE,
>>> +
>>> +	/**
>>> +	 * Write without waiting the data to be
>>> +	 * flushed to the storage device.
>>> +	 */
>>> +	WAL_WRITE,
>>> +
>>> +	/**
>>> +	 * Write data and wait the record to be
>>> +	 * flushed to the storage device.
>>> +	 */
>>> +	WAL_FSYNC,
>>> +
>>> +	WAL_MODE_MAX
>>> +};
>>> +
>>> +/** String constants for the supported modes. */
>>> +extern const char *wal_mode_STRS[WAL_MODE_MAX];
>> 1. Why do you need to declare in the header the array size?
>> The old version with [] was just fine. And less places to change
>> if becomes necessary.
> If the size is known better to point it explicitly. Thus when you
> read this code you will notise the array size (yes, it doesn't
> matter from compiler pov if we declare it as a double pointer
> but still). But fine I'll leave this declaration untouched.
>
>> 2. Please, keep the global variable where it was. We usually group
>> global variable declarations. It was grouped with wal_dir_lock
>> below.
> ---
> From: Cyrill Gorcunov <gorcunov@gmail.com>
> Date: Fri, 16 Apr 2021 15:54:52 +0300
> Subject: [PATCH] wal: sanitize wal_mode
>
>   - there is no need to set WAN_NONE to 0 explicitly
>   - add comments about modes
>   - there is no need to carry NULL in wal_mode_STRS
>   - use designated assignment in wal_mode_STRS
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/wal.c |  6 +++++-
>   src/box/wal.h | 21 ++++++++++++++++++++-
>   2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/src/box/wal.c b/src/box/wal.c
> index 5b6200b81..6468df884 100644
> --- a/src/box/wal.c
> +++ b/src/box/wal.c
> @@ -55,7 +55,11 @@ enum {
>   	WAL_FALLOCATE_LEN = 1024 * 1024,
>   };
>   
> -const char *wal_mode_STRS[] = { "none", "write", "fsync", NULL };
> +const char *wal_mode_STRS[WAL_MODE_MAX] = {
> +	[WAL_NONE]	= "none",
> +	[WAL_WRITE]	= "write",
> +	[WAL_FSYNC]	= "fsync",
> +};
>   
>   int wal_dir_lock = -1;
>   
> diff --git a/src/box/wal.h b/src/box/wal.h
> index a8a16ba2e..36fcd39bd 100644
> --- a/src/box/wal.h
> +++ b/src/box/wal.h
> @@ -41,7 +41,26 @@ struct fiber;
>   struct wal_writer;
>   struct tt_uuid;
>   
> -enum wal_mode { WAL_NONE = 0, WAL_WRITE, WAL_FSYNC, WAL_MODE_MAX };
> +enum wal_mode {
> +	/**
> +	 * Do not write data at all.
> +	 */
> +	WAL_NONE,
> +
> +	/**
> +	 * Write without waiting the data to be
> +	 * flushed to the storage device.
> +	 */
> +	WAL_WRITE,
> +
> +	/**
> +	 * Write data and wait the record to be
> +	 * flushed to the storage device.
> +	 */
> +	WAL_FSYNC,
> +
> +	WAL_MODE_MAX
> +};
>   

Thanks for the patch!

I'd leave an explicit WAL_NONE = 0.
I didn't know that enums start at zero, TBH.
This would be more readable IMO.

Up to you.

>   enum {
>   	/**

-- 
Serge Petrenko