Re: [PATCH] FileMap c++ refactoring

From: Kinkie <gkinkie_at_gmail.com>
Date: Thu, 24 Nov 2011 16:11:42 +0100

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

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

-- 
    /kinkie

Received on Thu Nov 24 2011 - 15:11:51 MST

This archive was generated by hypermail 2.2.0 : Sat Nov 26 2011 - 12:00:07 MST