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
