Re: [PATCH] FileMap c++ refactoring

>>>> +/** Compact bit-field representation class
>>>> + *
>>>> + * FileMap is aimed at holding UFS's internal file allocation table.
>>>
>>> I do not think FileMap is a compact representation of a bit-field, but
>>> perhaps you meant something else? Consider this clarification:
>>>
>>> /** A bitmap used for managing UFS StoreEntry "file numbers".
>>>  *
>>>  * Nth bit represents whether file number N is used.
>>>  * The map automatically grows to hold up to 2^24 bits.
>>>  * New bit is "off" or zero by default, representing unused fileno.
>>>  */
>>
>> Thanks. Done.
>>
>>>> +    /// Test whether the num-th bit in the FileMap is set
>>>> +    int testBit(int num) const;
>>>
>>> I would rename that to isUsed(), isSet() or similar and return bool
>>> instead of int, but that is not a big deal.
>>
>> Leaving as-is not to grow the scope.
>
>
> SourceLayout refactoring does include code style updates to the current
> guideline requirements. So I'm seconding that bool type usage please, if not
> the name part.

Done the type changing.

> in FileMap.h:
>  * max_n_files would be best called capacity_ or currentCapacity_
> internally. Should be unsigned too if that does not clash with sfileno
> comparisons.

It clashes. Changed to sfileno capacity_

>  * n_files_in_map would be best called something_ internally. Also this
> should be unsigned.

changed to sfileno used_slots_

>  * please use the available sfileno type instead of "int" for all variables
> representing the file number.

Ok.

> in FileMap.cc:
>  * please change the squid.h include to config.h and add other minimal
> includes as required to minimize the dependency.

Done.

>  * the allocate() loop iterators count and bit can be defined in their loop
> definitions.

Done.

>  * in allocate() "debugs(8, 3, HERE << "growing fileMap");" can die, it is
> covered by the message inside grow().

Done.

> in UFSSwapDir::mapBitReset
>  * s/calling resetting/calling clearBit/
>  * s/resetting doesn't/clearBit doesn't/

Done in a slightly different way.

> 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. 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.

-- 
    /kinkie