Re: [PATCH] FileMap c++ refactoring

On 11/23/2011 04:12 PM, Kinkie wrote:

> the attached patch is a direct c++ refactoring and documentation
> effort for struct fileMap (which gets renamed class FileMap).
> Scope is very limited; only ufs uses this code. Main benefit is the
> reduction in pollution in global include files.

> - fileMap *fm = (fileMap *)xcalloc(1, sizeof(fileMap));
...
> +FileMap::FileMap() {
> + max_n_files = FM_INITIAL_NUMBER;
> + nwords = max_n_files >> LONG_BIT_SHIFT;
> + debugs(8, 3, HERE << "creating space for " << max_n_files << " files");
> + debugs(8, 5, "--> " << nwords << " words of " << sizeof(*file_map) << " bytes each");
> + file_map = (unsigned long *)xcalloc(nwords, sizeof(*file_map));
> }

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.

> +/** 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.
  */

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

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

I would also s/file_map/words/ or similar (to avoid "file map inside a
file map" implication) but that is your call.

I would also add the following to-do, perhaps where file_map data member
is declared:

// TODO: Consider using std::bitset instead.

HTH,

Alex.
Received on Thu Nov 24 2011 - 00:57:57 MST