Re: [PATCH] Refactor ufscommon into individual include/code files

Hi,
  v2 addresses (or at least triest to) address all your suggestions,
plus it changes DBG_CRITICAL and DBG_IMPORTANT across the whole
source, and changes CBDATA_CLASS* positions everywhere.
Due to feeping creaturism the patch name is not really correct
anymore, but I'll stick to it for now. The relative feature-branch is
lp:~kinkie/squid/fixme-fixes .

On Wed, Aug 1, 2012 at 3:01 AM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On 01.08.2012 10:20, Kinkie wrote:
>>
>> Hi all,
>> the attached patch splits ufscommon.h and .cc into specific-purpose
>> files, as per squid standard, and reconnects the dots.
>> There are no functional changes, but it removes a few FIXMEs in the
>> code. Test-suite-tested.
>>
>> I'm not sure if this is the right time to merge, or if it's better to
>> wait after 3.2 ships, but here goes.
>
>
> The testRock breakage caused by r really needs to be fixed before its worth
> adding anything new to trunk. We will just end up piling in more minor
> undetected bugs.
>
> NP: Nothing but bug fixes for current build failures on FreeBSD, OpenBSD,
> Windows which we can test directly will get into 3.2 now until after
> release. it is 2 days overdue right now.
>
>
>
> If this passes the 3.ALPHA-matrix test scans I'm not too worried about it
> being applied to trunk. But as a refactor ("SourceLayout:" project) it is
> incomplete (namespace upgrade not done with appropriate symbol changes).
>
> ** please update the table in wiki SourceLayout page to track this progress
> and in what sub-dir the refactoring is underway in. Will need a new row for
> the fs/ufs/ specific component details
>
>
> Audit ...
>
> ... I have mentioned debugs changes, now would be a good time to do that
> for this code. but if you want to skip it this patch that is okay.
>
>
> src/Makefile.am:
> - irrelevant whitespace change - please revert.
>
>
> src/fs/ufs/RebuildState.cc:
>
> - irrelevant re-naming the file in its header comment. Please remove.
> - missing DEBUG section tag in the file header
> - missing AUTHOR tag in the file header (if you can identify original
> authors easily please add, otherwise this is fine)
> - extra whitespace line after CBDATA init statement. please remove.
> - debugs level-1 please update to DBG_IMPORTANT
> - debugs level-2 and lower please update to using HERE
> - debugs level-1+ please add WARNING:/ERROR:/ etc as appropriate
> - since this is a polish patch:
>
> + RebuildState::RebuildState
> - please elide SP between function name and parameter list initial "("
> - please ensure consistent use of whitespace around initializer list
> ie "sd(aSwapDir), LogParser(NULL), e(NULL), fromLog(true), _done(false)"
>
> src/fs/ufs/RebuildState.h:
> - please move class pre-defines down the #include. If the included files
> need them, they much be pre-defined in there as well.
> same for src/fs/ufs/UFSStrategy.h
>
>
> src/fs/ufs/StoreFSufs.cc:
> - it would seem "DiskIO/DiskIOModule.h" inside "#if 0" is useless, please
> remove that #if 0 section.
>
>
> src/fs/ufs/StoreSearchUFS.cc:
> - extra whitespace line after squid.h include. please remove. same for
> other files.
> - missing whitespace line after CBDATA init statement. please add
> - inconsistent use of whitespace in StoreSearchUFS::StoreSearchUFS
> definition, same as for RebuildState::RebuildState
>
>
> src/fs/ufs/StoreSearchUFS.h and src/fs/ufs/UFSStoreState.h and
> src/fs/ufs/UFSSwapDir.h and src/fs/ufs/UFSSwapLogParser.h
> - excess whitespace lines in file tail. please reduce to one empty line.
> same for other .h files
> - extra line before "public:". same for a lot of the class definitions.
> please remove
> - src/fs/ufs/UFSSwapLogParser.h also has class predefine before #includes
>
>
> src/fs/ufs/UFSSwapDir.cc:
> - please remove double whitespace lines between definitions.
> - please remove empty first-line inside function definitions.
> - please fix documentation comments on all functions to doxygen-style and
> omit obsolete symbol names
> - please fix whitespace alignment on initializer list of
> UFSSwapDir::UFSSwapDir, also please make it a vertical list 8-space indented
> - debugs level 0 and 1, please prefix with WARNING/ERROR as appropriate and
> use HERE on level-2+
>
> src/fs/ufs/UFSSwapLogParser.cc:
> - please remove excess empty lines: one after squid.h, one after class
> definitions, several at file end.
>
> src/fs/ufs/store_dir_ufs.cc:
> - appears to have been left containing a useless #define. Please remove
> that.
>
> src/tests/testUfs.cc:
> - please remove excess whitespace line after squid.h
>
>
> Across the board:
> - please add \bug comments in all classes where CBDATA_CLASS definition is
> not last in the class{} definition. The macro plays tricks with
> public/private which screws up what the .h file *looks* like it is defining
> things as.
>
> There is a couple of major TODO missing in those spots as well:
> // TODO: move CBDATA_CLASS macro down to last as ensure the classes still
> build fine (nothing was silently depending on the functions/members defined
> after it being public:).
>
>
>
> Amos
>

-- 
    /kinkie