Re: [PATCH] FileMap c++ refactoring

From: Kinkie <gkinkie_at_gmail.com>
Date: Mon, 28 Nov 2011 08:15:35 +0100

>> changed to sfileno used_slots_
>>
>
> Err... camelCase_
>
> <snip>

Whoops, sorry. I thought that the convention didn't apply to private
data members.

>>> 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.
>
>
> Okay, up to you.
>
> With those mentioned changes its passed me.
>
>
>
>> 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.
>
>
> I would not really call that "suggestion" variable layer breaking. UFS is
> not manipulating it directly AFAICS. Just passing back to the FileMap later.
> Which IMO is a correct implementation of the temporality optimization.
>
> The StoreMap could be considered a layer break in a certain view. I agree
> that is way out of scope here though. So far out I'd argue its out of scope
> of improving FileMap too.

As FileMap is not merely a bitfield, I believe that the suggestion
would be better handled in FileMap itself, in fact then it wouldn't be
an optimization at all but a class-managed private data member, and
FileMap would then offer the service of being a "file allocation
table" of sorts, with a cleaner interface (set bit N, clear bit N,
find any unset bit set it and return its position), which now it kind
of sort of is but in order to be efficient it requires the
collaboration of the caller (see suggest, which can very easily be a
private data member of FileMap).

Anyways, may I assume that I have go-ahead for commit provided that I
change the camelcase and add TODOs about this discussion?

Thanks

-- 
    /kinkie
Received on Mon Nov 28 2011 - 07:15:42 MST

This archive was generated by hypermail 2.2.0 : Mon Nov 28 2011 - 12:00:07 MST