Re: [PATCH] FileMap c++ refactoring

On 25/11/2011 4:11 a.m., Kinkie wrote:
>> The above change leaves n_files_in_map uninitialized.
>>
>> In general, please initialize members before entering the body of the
>> constructor. This will save you from bugs such as this one and will also
>> make the code more exception-safe.
> Done.
>
>>> +/** 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.

>
>>> + // max number of files which can be tracked in the current store
>>> + int max_n_files;
>>> + int n_files_in_map; // used slots in the map
>>> + int nwords; // number of "long ints" making up the filemap
>>> + unsigned long *file_map;
>> Please use doxygen comments for data members and document file_map as well.
> Done.
>
>> I would also s/file_map/words/ or similar (to avoid "file map inside a
>> file map" implication) but that is your call.
> Renamed as "bitmap". Is that OK?
>
>> I would also add the following to-do, perhaps where file_map data member
>> is declared:
>>
>> // TODO: Consider using std::bitset instead.
> Done.
>
> Version 2 attached.
> Thanks
>

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.
  * n_files_in_map would be best called something_ internally. Also this
should be unsigned.
  * please use the available sfileno type instead of "int" for all
variables representing the file number.

in FileMap.cc:
  * please change the squid.h include to config.h and add other minimal
includes as required to minimize the dependency.
  * the allocate() loop iterators count and bit can be defined in their
loop definitions.
  * in allocate() "debugs(8, 3, HERE << "growing fileMap");" can die, it
is covered by the message inside grow().

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

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.

Amos
Received on Sat Nov 26 2011 - 03:07:38 MST