Re: [PATCH] FileMap c++ refactoring

 On Sun, 27 Nov 2011 19:55:51 +0100, Kinkie wrote:
 <snip>
>>  * n_files_in_map would be best called something_ internally. Also
>> this
>> should be unsigned.
>
> changed to sfileno used_slots_
>

 Err... camelCase_

 <snip>

>
>> NP: also this would be a good time to add that bounds checking and
>> ensure
>> the n_files_in_map is correctly maintained by its owner class.
>> Especially
>> since its a private. This goes for both setBit() and checkBit(). The
>> return
>> value from setBit() appears not to be used anywhere,  so both can
>> change to
>> bool and report back success/fail to the caller in case it is ever
>> needed.
>
> This would go in the "improvements" part.

 Okay, up to you.

 With those mentioned changes its passed me.

> Also, there is a bit of
> layering breaking in UFSSwapDir where it has its own view of the map,
> including e.g. keeping a cached "last allocated" index to speed
> allocations up.
>
> I'd prefer not to address these at this time, but I could add a TODO
> to remind about them if you wish.
> V3 attached.

 I would not really call that "suggestion" variable layer breaking. UFS
 is not manipulating it directly AFAICS. Just passing back to the FileMap
 later. Which IMO is a correct implementation of the temporality
 optimization.

 The StoreMap could be considered a layer break in a certain view. I
 agree that is way out of scope here though. So far out I'd argue its out
 of scope of improving FileMap too.

 Amos
Received on Mon Nov 28 2011 - 00:36:52 MST