Re: [PATCH] complete the StoreEntry and StoreIOState constructors

On 12/30/2012 10:37 AM, Kinkie wrote:
> StoreEntry and StoreIOState's constructors don't initialize all
> their data members; the layering is currently not very robust and
> initialization relies on the caller.
> This patch attempts to fill in all data members for those classes for
> which I could find a reasonable default

> + // note: refcount is actually set in storeCreateEntry
...
> + // note: refcount is actually set in storeCreateEntry

Please remove these new comments. They are wrong (you probably meant
lockcount and not refcount) and, even if that problem is fixed, they are
going to be misleading. There are many ways to create a StoreEntry, and
these comments may mislead folks to think that storeCreateEntry() is the
only avenue to entry creation or recount setting.

> === modified file 'src/StoreIOState.h'
> --- src/StoreIOState.h 2012-10-29 04:59:58 +0000
> +++ src/StoreIOState.h 2012-12-30 17:26:04 +0000
> @@ -105,7 +105,7 @@
> } read;
>
> struct {
> - unsigned int closing:1; /* debugging aid */
> + bool closing:1; /* debugging aid */
> } flags;
> };
>

Let's just use "bool" here, without downsizing it. I am not sure
downsizing "bool" is 100% legitimate and, even if it is, it is not going
to save any RAM due to padding (and may introduce CPU overheads).

I did not notice any more problems. Please feel free to commit a
polished version if there are no other opinions.

Thank you,

Alex.
Received on Mon Dec 31 2012 - 21:02:04 MST